{"thread":{"id":"23143","subject":"Question about .git/objects/info/alternates","startedAt":"2010-03-22T17:26:05Z","lastAt":"2010-03-31T04:35:16Z","messageCount":17,"participants":["Chris Packham","Jonathan Nieder","Junio C Hamano","Stephen Boyd","Johannes Sixt","Miklos Vajna"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"137572","messageId":"a038bef51003221026i379ee16ej6e0e6defcf0048dd@mail.gmail.com","threadId":"23143","inReplyTo":null,"subject":"Question about .git/objects/info/alternates","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2010-03-22T17:26:05Z","receivedAt":"2010-03-22T17:26:05Z","isPatch":false,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"Hi All,\n\nI've been using git clone --reference and git submodule update\n--reference to reduce the amount of data transferred when I cloned a\nrepository that I already had an older copy of (that for one reason or\nanother I didn't want to touch or clone directly).\n\nNow I'm finding that what I really want to do is change around what is\nreferencing what. I currently have the following.\n\n  projecta.git\n  base.git       # references project a\n  projectb.git  # referenced base (which, now that I think about it,\nwas probably the wrong thing to do)\n\nIdeally I'd want to end up with\n  base.git      # has all objects\n  projecta.git # uses base as a reference\n  projectb.git # uses base as a reference also\n\nI would like to have base somehow find the objects it doesn't have in\nits object store and either download them or just copy them from the\nobject store of projecta. Then I can manually point projecta at base\nand repack (as discussed in this thread [1]) to free up some space.\nprojectb should be fine as is because it already references base. Is\nthere any way to actually do this? A little googling found hints on\nadding alternates after the fact but I'm actually interested in going\nthe other direction. From reading [2] I think 'rm\n.git/objects/info/alternates && git repack -a' might do the trick but\nI'm not sure.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/141161/focus=141199\n[2] http://stackoverflow.com/questions/2248228/how-to-detach-alternates-after-git-clone-reference\n"},{"id":"137604","messageId":"20100323024223.GA12257@progeny.tock","threadId":"23143","inReplyTo":"a038bef51003221026i379ee16ej6e0e6defcf0048dd@mail.gmail.com","subject":"Re: Question about .git/objects/info/alternates","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-03-23T02:42:23Z","receivedAt":"2010-03-23T02:42:23Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Chris Packham wrote:\n\n> I would like to have base somehow find the objects it doesn't have in\n> its object store and either download them or just copy them from the\n> object store of projecta.\n[...]\n> From reading [2] I think 'rm\n> .git/objects/info/alternates && git repack -a' might do the trick but\n> I'm not sure.\n\nAlmost.  Try ‘git repack -a && rm .git/objects/info/alternates’ instead. :)\n\n(Please back up the repository or try with something less important\nfirst, since I am not sure.)\n\nHope that helps,\nJonathan\n\n> [2] http://stackoverflow.com/questions/2248228/how-to-detach-alternates-after-git-clone-reference\n"},{"id":"137721","messageId":"a038bef51003241153g33445607qb3ab750e08b0584@mail.gmail.com","threadId":"23143","inReplyTo":"20100323024223.GA12257@progeny.tock","subject":"Re: Question about .git/objects/info/alternates","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2010-03-24T18:53:08Z","receivedAt":"2010-03-24T18:53:08Z","isPatch":false,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"On Mon, Mar 22, 2010 at 7:42 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Chris Packham wrote:\n>\n>> I would like to have base somehow find the objects it doesn't have in\n>> its object store and either download them or just copy them from the\n>> object store of projecta.\n> [...]\n>> From reading [2] I think 'rm\n>> .git/objects/info/alternates && git repack -a' might do the trick but\n>> I'm not sure.\n>\n> Almost.  Try ‘git repack -a && rm .git/objects/info/alternates’ instead. :)\n>\n> (Please back up the repository or try with something less important\n> first, since I am not sure.)\n>\n> Hope that helps,\n> Jonathan\n>\n\ngit repack -a did the correct thing.\n\nIt occurs to me that the UI around alternates is a bit lacking i.e.\nthere isn't a git command to display the alternates in use or to add\nthem to an existing repository (or at least I couldn't find one\nskimming the docs or googling). So here's my attempt to add a 'git\nalternates' command which can display, add or remove an alternate. The\nadding and removing could be done with standard shell commands but\nI've found the recursive displaying quite useful and couldn't think of\na simple command line to achieve the same thing .\n\n------8<------\n\nFrom a5c64de20937da132376d717f19a1d52b54701d2 Mon Sep 17 00:00:00 2001\nFrom: Chris Packham <judge.packham@gmail.com>\nDate: Wed, 24 Mar 2010 11:34:11 -0700\nSubject: [PATCH] Add git alternates command\n\nProvides a friendlier UI for displaying and configuring alternates.\n\nSigned-off-by: Chris Packham <judge.packham@gmail.com>\n---\n\nThis patch assumes multiple alterates are possible. If this is not correct then\nit could be simplfied as there would be no need for walk_alternates.\n\n Makefile          |    1 +\n git-alternates.sh |  159 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 160 insertions(+), 0 deletions(-)\n create mode 100755 git-alternates.sh\n\ndiff --git a/Makefile b/Makefile\nindex 3a6c6ea..1a7b084 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -334,6 +334,7 @@ TEST_PROGRAMS_NEED_X =\n unexport CDPATH\n\n SCRIPT_SH += git-am.sh\n+SCRIPT_SH += git-alternates.sh\n SCRIPT_SH += git-bisect.sh\n SCRIPT_SH += git-difftool--helper.sh\n SCRIPT_SH += git-filter-branch.sh\ndiff --git a/git-alternates.sh b/git-alternates.sh\nnew file mode 100755\nindex 0000000..74ec707\n--- /dev/null\n+++ b/git-alternates.sh\n@@ -0,0 +1,159 @@\n+#!/bin/sh\n+#\n+# This file is licensed under the GPL v2\n+#\n+\n+USAGE='[-r|--recursive] [-a|--add <dir>] [-f|--force -d|--delete <dir>]'\n+\n+. git-sh-setup\n+\n+#\n+# Runs through the alternates file calling the callback function $1\n+# with the name of the alternate as the first argument to the callback\n+# any additional arguments are passed to the callback function.\n+#\n+walk_alternates()\n+{\n+    local alternates=$GIT_DIR/objects/info/alternates\n+    local callback=$1\n+    shift\n+\n+    if [ -e $alternates ]; then\n+        while read line\n+        do\n+            $callback $line $*\n+        done < $alternates\n+    fi\n+}\n+\n+#\n+# Walk function to display one alternate object store and, if the user\n+# has specified -r, recursively call show_alternates on the git\n+# repository that the object store belongs to.\n+#\n+show_alternates_walk()\n+{\n+    say \"Object store $1\"\n+    say \"    referenced via $GIT_DIR\"\n+\n+    local new_git_dir=${line%%/objects}\n+    if [ \"$recursive\" == \"true\" -a \"$GIT_DIR\" != \"$new_git_dir\" ]\n+    then\n+        GIT_DIR=$new_git_dir show_alternates\n+    fi\n+}\n+\n+show_alternates()\n+{\n+    walk_alternates show_alternates_walk\n+}\n+\n+#\n+# Walk function to check that the specified alternate does not\n+# already exist.\n+#\n+check_current_alternate_walk()\n+{\n+    if test \"$1\" = \"$2\"; then\n+        die \"fatal: Object store $2 is already used by $GIT_DIR\"\n+    fi\n+}\n+\n+add_alternate()\n+{\n+    if test ! -d $dir; then\n+        die \"fatal: $dir is not a directory\"\n+    fi\n+\n+    walk_alternates check_current_alternate_walk $dir\n+\n+    # At this point we know that $dir is a directory that exists\n+    # and that its not already being used as an alternate. We could\n+    # go further and verify that $dir has valid objects.\n+\n+    # if we're still going we can safely add the alternate\n+    touch $GIT_DIR/objects/info/alternates\n+    echo \"$(readlink -f $dir)\" >> $GIT_DIR/objects/info/alternates\n+    say \"$dir added as an alternate\"\n+    say \"     use 'git repack -adl' to remove duplicate objects\"\n+}\n+\n+rewrite_alternates()\n+{\n+    if test \"$1\" != \"$2\"; then\n+        echo $2 >> $3\n+    fi\n+}\n+\n+del_alternate()\n+{\n+    if test ! $force = \"true\"; then\n+        say \"Not forced, use\"\n+        say \"   'git repack -a' to fetch missing objects, then \"\n+        say \"   '$dashless -f -d $dir' to remove the alternate\"\n+        die\n+    fi\n+\n+    local alternates=$GIT_DIR/objects/info/alternates\n+\n+    new_alts_file=$(mktemp $alternates-XXXXXX)\n+    touch $new_alts_file\n+\n+    walk_alternates rewrite_alternates $dir $new_alts_file\n+    mv $new_alts_file $alternates\n+\n+    # save the git from repeatedly reading a 0 length file\n+    if test $(stat -c \"%s\" $alternates) -eq 0; then\n+        rm $alternates\n+    fi\n+}\n+\n+dir=\"\"\n+oper=\"\"\n+force=\"false\"\n+\n+# Option parsing\n+while test $# != 0\n+do\n+    case \"$1\" in\n+        -r|--recursive)\n+            recursive=\"true\"\n+            ;;\n+        -a|--add)\n+            if test ! -z \"$oper\"; then\n+                usage\n+            fi\n+            oper=\"add\"\n+            case \"$#,$1\" in\n+                1,*) usage ;;\n+                *)   dir=$2; shift ;;\n+            esac\n+            ;;\n+        -d|--delete)\n+            if test ! -z \"$oper\"; then\n+                usage\n+            fi\n+            oper=\"del\"\n+            case \"$#,$1\" in\n+                1,*) usage ;;\n+                *)   dir=$2; shift ;;\n+            esac\n+            ;;\n+        -f|--force)\n+            force=\"true\"\n+            ;;\n+        -*)\n+            usage\n+            ;;\n+        *)\n+            ;;\n+    esac\n+    shift\n+done\n+\n+# Now go and do it\n+case $oper in\n+    add) add_alternate ;;\n+    del) del_alternate ;;\n+    *)   show_alternates ;;\n+esac\n-- \n1.7.0.3\n"},{"id":"137726","messageId":"20100324192315.GA19322@progeny.tock","threadId":"23143","inReplyTo":"a038bef51003241153g33445607qb3ab750e08b0584@mail.gmail.com","subject":"Re: Question about .git/objects/info/alternates","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-03-24T19:23:15Z","receivedAt":"2010-03-24T19:23:15Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Chris Packham wrote:\n\n> It occurs to me that the UI around alternates is a bit lacking i.e.\n> there isn't a git command to display the alternates in use or to add\n> them to an existing repository (or at least I couldn't find one\n> skimming the docs or googling). So here's my attempt to add a 'git\n> alternates' command which can display, add or remove an alternate.\n\nQuite welcome!\n\nSomething like this explanation probably belongs in the commit\nmessage.  That way, years down the line, people don’t need to trawl\nthe list archives to see what your goal was.\n\nComments:\n\n> From a5c64de20937da132376d717f19a1d52b54701d2 Mon Sep 17 00:00:00 2001\n> From: Chris Packham <judge.packham@gmail.com>\n> Date: Wed, 24 Mar 2010 11:34:11 -0700\n\nRedundant next to the email header.  It is useful to be able to include\nfields that differ from the e-mail header (often Subject:,\nsometimes Date:, sometimes From:), but aside from that, this metadata\nshould be omitted when sending patches to the git mailing list.\n\n> +#\n> +# Runs through the alternates file calling the callback function $1\n> +# with the name of the alternate as the first argument to the callback\n> +# any additional arguments are passed to the callback function.\n> +#\n> +walk_alternates()\n> +{\n> +    local alternates=$GIT_DIR/objects/info/alternates\n> +    local callback=$1\n\nI couldn’t find any other uses of “local” in-tree.  I assume old shells\ndon’t support it.\n\nWill this be a problem for recursion?  Maybe the callback should be\ncalled in a subshell.\n\n> +    shift\n> +\n> +    if [ -e $alternates ]; then\n> +        while read line\n> +        do\n> +            $callback $line $*\n\nProbably \"$line\" \"$@\" instead of $line $* would be more flexible.\n\n> +#\n> +# Walk function to display one alternate object store and, if the user\n> +# has specified -r, recursively call show_alternates on the git\n> +# repository that the object store belongs to.\n> +#\n> +show_alternates_walk()\n> +{\n> +    say \"Object store $1\"\n> +    say \"    referenced via $GIT_DIR\"\n> +\n> +    local new_git_dir=${line%%/objects}\n\nuse of local.\n\n> +    if [ \"$recursive\" == \"true\" -a \"$GIT_DIR\" != \"$new_git_dir\" ]\n\nShould use = instead of == (portability).  Also git scripts tend to\nspell out 'test' and avoid the -a and -o operators:\n\n\tif test \"$recursive\" = true && test \"$GIT_DIR\" != \"$new_git-dir\"\n\nthough that is not a hard and fast rule.\n\n> +add_alternate()\n> +{\n[...]\n> +    touch $GIT_DIR/objects/info/alternates\n\nNecessary?\n\n> +    echo \"$(readlink -f $dir)\" >> $GIT_DIR/objects/info/alternates\n\nMaybe\n\n\treadlink -f \"$dir\" >> $GIT_DIR/objects/info/alternates\n\nwould be simpler.\n\n> +rewrite_alternates()\n> +{\n> +    if test \"$1\" != \"$2\"; then\n> +        echo $2 >> $3\n> +    fi\n> +}\n\nWhat does this function do?  (Could use a comment.)\n\n> +del_alternate()\n> +{\n[...]\n> +    local alternates=$GIT_DIR/objects/info/alternates\n\nuse of local.\n\n> +\n> +    new_alts_file=$(mktemp $alternates-XXXXXX)\n\nNot used elsewhere in git.  Is this needed?  Maybe a single\n$GIT_DIR/objects/info/new-alternates.tmp or similar would be good\nenough.\n\n> +    # save the git from repeatedly reading a 0 length file\n> +    if test $(stat -c \"%s\" $alternates) -eq 0; then\n\nNot used elsewhere in core git.  test -s can help.\n\n> +# Option parsing\n\nSee OPTIONS_SPEC in git-repack.sh for an example of how to simplify\nthis.\n\n> +# Now go and do it\n> +case $oper in\n> +    add) add_alternate ;;\n> +    del) del_alternate ;;\n> +    *)   show_alternates ;;\n> +esac\n\nThank you for your excellent work!  Looks very useful.\n\nRegards,\nJonathan\n"},{"id":"137729","messageId":"7vljdhh4po.fsf@alter.siamese.dyndns.org","threadId":"23143","inReplyTo":"a038bef51003241153g33445607qb3ab750e08b0584@mail.gmail.com","subject":"Re: Question about .git/objects/info/alternates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-24T19:58:27Z","receivedAt":"2010-03-24T19:58:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Packham <judge.packham@gmail.com> writes:\n\n> git repack -a did the correct thing.\n>\n> It occurs to me that the UI around alternates is a bit lacking i.e.\n> there isn't a git command to display the alternates in use or to add\n> them to an existing repository (or at least I couldn't find one\n> skimming the docs or googling).\n\nThat's an understatement.  Thanks for starting the effort.\n\nI will likely to have quite a few style issues with the script\nimplementation, and am undecided if this should be a new command\nor an option to some existing command, but it's time we have a\nmanagement facility for alternates.\n\n> diff --git a/Makefile b/Makefile\n> index 3a6c6ea..1a7b084 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -334,6 +334,7 @@ TEST_PROGRAMS_NEED_X =\n>  unexport CDPATH\n>\n>  SCRIPT_SH += git-am.sh\n> +SCRIPT_SH += git-alternates.sh\n>  SCRIPT_SH += git-bisect.sh\n>  SCRIPT_SH += git-difftool--helper.sh\n>  SCRIPT_SH += git-filter-branch.sh\n\nYou probably need one entry in the command-list.txt to classify where in\nthe main manual page git(1) for this command to appear.  I would suggest\nimitating \"git config\".\n\n> diff --git a/git-alternates.sh b/git-alternates.sh\n> new file mode 100755\n> index 0000000..74ec707\n> --- /dev/null\n> +++ b/git-alternates.sh\n> @@ -0,0 +1,159 @@\n> +#!/bin/sh\n> ...\n> +#\n> +# Runs through the alternates file calling the callback function $1\n> +# with the name of the alternate as the first argument to the callback\n> +# any additional arguments are passed to the callback function.\n> +#\n> +walk_alternates()\n> +{\n> +    local alternates=$GIT_DIR/objects/info/alternates\n> +    local callback=$1\n\nWe want to use this on platforms with ksh and dash, so please refrain from\nbash-ism features.  \"local\" does not mix well with \"#!/bin/sh\".\n\n> +    shift\n> +\n> +    if [ -e $alternates ]; then\n\nWe tend to avoid '[' and write it like this:\n\n\tif test -f \"$alternates\"\n        then\n        \t...\n\nAlso notice that an indent is one tab and one tabstop is 8 places, and\nalternates need to be quoted in case $GIT_DIR has IFS whitespace in it.\n\n> +        while read line\n> +        do\n> +            $callback $line $*\n\nAs the path to an alternate object store can contain IFS whitespace, line\nneeds to be quoted.  Also you call walk_alternates with things like \"$dir\" \nthat could also be a path with IFS whitespace, it needs to be quoted\nproperly.  I suspect callback is only your own shell function, so it does\nnot have to be quoted, but it is Ok to quote it for consistency.  I.e.\n\n\t\"$callback\" \"$line\" \"$@\"\n\nYou are loooooose in quoting throughout your script, so I won't bother to\npoint all of them out in the rest of the message.  You also are loose in\nchecking error returns (what you failed to write $newalternates file, for\nexample), which needs to be fixed before the final version.\n\n> +        done < $alternates\n\nThis is correct per POSIX even if $alternates itself has IFS whitespace in\nit, but newer bash on some platforms have an obnoxious \"safety feature\"\n(see 3fa7c3d work around an obnoxious bash \"safety feature\" on OpenBSD,\n2010-01-26) that can cause it to barf on this.  It is unfortunate but we\nneed to quote it like this:\n\n\tdone <\"$alternates\"\n\nAlso notice that there is no SP after < or > redirection (it is just a\nstyle thing).\n\n> +# Walk function to display one alternate object store and, if the user\n> +# has specified -r, recursively call show_alternates on the git\n> +# repository that the object store belongs to.\n> +#\n> +show_alternates_walk()\n> +{\n> +    say \"Object store $1\"\n> +    say \"    referenced via $GIT_DIR\"\n> +\n> +    local new_git_dir=${line%%/objects}\n> +    if [ \"$recursive\" == \"true\" -a \"$GIT_DIR\" != \"$new_git_dir\" ]\n> +    then\n\n\tif test \"$recursive\" = true && test \"$GIT_DIR\" != \"$new_git_dir\"\n        then\n        \t...\n\n> +        GIT_DIR=$new_git_dir show_alternates\n\nThis is probably depending on a bug in bash and is not portable.  See\n76c9c0d (rebase -i: Export GIT_AUTHOR_* variables explicitly, 2010-01-22)\n\n> +add_alternate()\n> +{\n> +    if test ! -d $dir; then\n> +        die \"fatal: $dir is not a directory\"\n> +    fi\n\nThis will not work with relative alternates.  In two repositories A and B,\nwhere B borrows from A:\n\n\tA/.git/objects/\n        B/.git/objects/info/alternates\n\nthe alternates file in B can point at A's .git/objects, relative to its\nown .git/objects/.\n\n> +    walk_alternates check_current_alternate_walk $dir\n> +\n> +    # At this point we know that $dir is a directory that exists\n> +    # and that its not already being used as an alternate. We could\n> +    # go further and verify that $dir has valid objects.\n> +\n> +    # if we're still going we can safely add the alternate\n> +    touch $GIT_DIR/objects/info/alternates\n> +    echo \"$(readlink -f $dir)\" >> $GIT_DIR/objects/info/alternates\n\nWhat is this touch for?\n\nIs readlink(1) portable enough across platforms?  A more fundamental\nquestion is if resolving symbolic link like this behind user is a sensible\nthing to do, especially as you are ...\n\n> +    say \"$dir added as an alternate\"\n\n... lying to the user here, if $dir indeed is a symbolic link.\n\n> +rewrite_alternates()\n> +{\n> +    if test \"$1\" != \"$2\"; then\n> +        echo $2 >> $3\n> +    fi\n> +}\n\nThat's a misleading name for this helper function.\n\n> +del_alternate()\n> +{\n> +    if test ! $force = \"true\"; then\n> +        say \"Not forced, use\"\n> +        say \"   'git repack -a' to fetch missing objects, then \"\n> +        say \"   '$dashless -f -d $dir' to remove the alternate\"\n> +        die\n> +    fi\n> +\n> +    local alternates=$GIT_DIR/objects/info/alternates\n> +\n> +    new_alts_file=$(mktemp $alternates-XXXXXX)\n> +    touch $new_alts_file\n> +\n> +    walk_alternates rewrite_alternates $dir $new_alts_file\n\nI think this is a more expensive way to say:\n\n\tgrep -v -F \"$dir\" <\"$alternates\" >\"$new_alternates\"\n\n> +    mv $new_alts_file $alternates\n> +\n> +    # save the git from repeatedly reading a 0 length file\n> +    if test $(stat -c \"%s\" $alternates) -eq 0; then\n> +        rm $alternates\n> +    fi\n\nGood point, but it would be better to do something like this:\n\n\tgrep -v -F \"$dir\" <\"$alternates\" >\"$new_alternates\"\n\tif test -s \"$new_alternates\"\n        then\n        \tmv \"$new_alternates\" \"$alternates\"\n\telse\n        \trm -f \"$alternates\"\n\tfi\n\nwithout making it less portable by using \"stat\".\n"},{"id":"137733","messageId":"780e0a6b1003241316i4bbd4489w63ba0308706e4d20@mail.gmail.com","threadId":"23143","inReplyTo":"a038bef51003241153g33445607qb3ab750e08b0584@mail.gmail.com","subject":"Re: Question about .git/objects/info/alternates","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2010-03-24T20:16:05Z","receivedAt":"2010-03-24T20:16:05Z","isPatch":false,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"On Wed, Mar 24, 2010 at 11:53 AM, Chris Packham <judge.packham@gmail.com> wrote:\n>\n> +USAGE='[-r|--recursive] [-a|--add <dir>] [-f|--force -d|--delete <dir>]'\n[...]\n> +case $oper in\n> +    add) add_alternate ;;\n> +    del) del_alternate ;;\n> +    *)   show_alternates ;;\n> +esac\n\nFrom a very high-level this should probably be more like git remote\nand git notes. 'add' and 'delete' would be subcommands instead of\noptions. Plus you might have an explicit subcommand for show (or\nlist?). Something like\n\ngit alternates [show]  [-r|--recursive]\ngit alternates add <dir>\ngit alternates delete  [-f|--force] <dir>\n"},{"id":"137739","messageId":"a038bef51003241335l1623ade4i4a9b7269546739d4@mail.gmail.com","threadId":"23143","inReplyTo":"7vljdhh4po.fsf@alter.siamese.dyndns.org","subject":"Re: Question about .git/objects/info/alternates","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2010-03-24T20:35:25Z","receivedAt":"2010-03-24T20:35:25Z","isPatch":false,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"On Wed, Mar 24, 2010 at 12:58 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Chris Packham <judge.packham@gmail.com> writes:\n>\n>> git repack -a did the correct thing.\n>>\n>> It occurs to me that the UI around alternates is a bit lacking i.e.\n>> there isn't a git command to display the alternates in use or to add\n>> them to an existing repository (or at least I couldn't find one\n>> skimming the docs or googling).\n>\n> That's an understatement.  Thanks for starting the effort.\n\nThanks for the comments (and lessons in portable shell scripting).\nI'll re-roll shortly. Some responses to specific questions below.\n\n> I will likely to have quite a few style issues with the script\n> implementation, and am undecided if this should be a new command\n> or an option to some existing command, but it's time we have a\n> management facility for alternates.\n\nFair enough about style issues, I tend to code everything like its C I\nguess some habits are hard to break. I was wondering about the\njustification for having a separate command but I couldn't think of\nanywhere else it'd fit. Also calling the command alternates when we\ncreate them with git clone --reference seems a bit funny.\n\n>> diff --git a/Makefile b/Makefile\n>> index 3a6c6ea..1a7b084 100644\n>> --- a/Makefile\n>> +++ b/Makefile\n>> @@ -334,6 +334,7 @@ TEST_PROGRAMS_NEED_X =\n>>  unexport CDPATH\n>>\n>>  SCRIPT_SH += git-am.sh\n>> +SCRIPT_SH += git-alternates.sh\n>>  SCRIPT_SH += git-bisect.sh\n>>  SCRIPT_SH += git-difftool--helper.sh\n>>  SCRIPT_SH += git-filter-branch.sh\n>\n> You probably need one entry in the command-list.txt to classify where in\n> the main manual page git(1) for this command to appear.  I would suggest\n> imitating \"git config\".\n\nYes will do when I write the documentation\n\n>> diff --git a/git-alternates.sh b/git-alternates.sh\n>> new file mode 100755\n>> index 0000000..74ec707\n>> --- /dev/null\n>> +++ b/git-alternates.sh\n>> @@ -0,0 +1,159 @@\n>> +#!/bin/sh\n>> ...\n>> +#\n>> +# Runs through the alternates file calling the callback function $1\n>> +# with the name of the alternate as the first argument to the callback\n>> +# any additional arguments are passed to the callback function.\n>> +#\n>> +walk_alternates()\n>> +{\n>> +    local alternates=$GIT_DIR/objects/info/alternates\n>> +    local callback=$1\n>\n> We want to use this on platforms with ksh and dash, so please refrain from\n> bash-ism features.  \"local\" does not mix well with \"#!/bin/sh\".\n>\n>> +    shift\n>> +\n>> +    if [ -e $alternates ]; then\n>\n> We tend to avoid '[' and write it like this:\n>\n>        if test -f \"$alternates\"\n>        then\n>                ...\n\nI think there were a few of these I missed before submitting. Easy to fix.\n\n> Also notice that an indent is one tab and one tabstop is 8 places, and\n> alternates need to be quoted in case $GIT_DIR has IFS whitespace in it.\n\nI must have used the wrong thing as an example. I actually started\nwith tabs and converted it to spaces after looking at\ngit-mergetool.sh.\n\n>> +        while read line\n>> +        do\n>> +            $callback $line $*\n>\n> As the path to an alternate object store can contain IFS whitespace, line\n> needs to be quoted.  Also you call walk_alternates with things like \"$dir\"\n> that could also be a path with IFS whitespace, it needs to be quoted\n> properly.  I suspect callback is only your own shell function, so it does\n> not have to be quoted, but it is Ok to quote it for consistency.  I.e.\n>\n>        \"$callback\" \"$line\" \"$@\"\n>\n> You are loooooose in quoting throughout your script, so I won't bother to\n> point all of them out in the rest of the message.  You also are loose in\n> checking error returns (what you failed to write $newalternates file, for\n> example), which needs to be fixed before the final version.\n\nI'll fix these.\n\n>> +        done < $alternates\n>\n> This is correct per POSIX even if $alternates itself has IFS whitespace in\n> it, but newer bash on some platforms have an obnoxious \"safety feature\"\n> (see 3fa7c3d work around an obnoxious bash \"safety feature\" on OpenBSD,\n> 2010-01-26) that can cause it to barf on this.  It is unfortunate but we\n> need to quote it like this:\n>\n>        done <\"$alternates\"\n>\n> Also notice that there is no SP after < or > redirection (it is just a\n> style thing).\n>\n>> +# Walk function to display one alternate object store and, if the user\n>> +# has specified -r, recursively call show_alternates on the git\n>> +# repository that the object store belongs to.\n>> +#\n>> +show_alternates_walk()\n>> +{\n>> +    say \"Object store $1\"\n>> +    say \"    referenced via $GIT_DIR\"\n>> +\n>> +    local new_git_dir=${line%%/objects}\n>> +    if [ \"$recursive\" == \"true\" -a \"$GIT_DIR\" != \"$new_git_dir\" ]\n>> +    then\n>\n>        if test \"$recursive\" = true && test \"$GIT_DIR\" != \"$new_git_dir\"\n>        then\n>                ...\n>\n>> +        GIT_DIR=$new_git_dir show_alternates\n>\n> This is probably depending on a bug in bash and is not portable.  See\n> 76c9c0d (rebase -i: Export GIT_AUTHOR_* variables explicitly, 2010-01-22)\n\nOK I thought that was a standard thing. Exporting could play havoc\nwith recursion, will have to look for a solution for that.\n\n>> +add_alternate()\n>> +{\n>> +    if test ! -d $dir; then\n>> +        die \"fatal: $dir is not a directory\"\n>> +    fi\n>\n> This will not work with relative alternates.  In two repositories A and B,\n> where B borrows from A:\n>\n>        A/.git/objects/\n>        B/.git/objects/info/alternates\n>\n> the alternates file in B can point at A's .git/objects, relative to its\n> own .git/objects/.\n\nSo would the best approach be not to validate the input or to validate\nit relative to $PWD and .git/objects/. I think the answer ties into my\nuse of readlink.\n\n>> +    walk_alternates check_current_alternate_walk $dir\n>> +\n>> +    # At this point we know that $dir is a directory that exists\n>> +    # and that its not already being used as an alternate. We could\n>> +    # go further and verify that $dir has valid objects.\n>> +\n>> +    # if we're still going we can safely add the alternate\n>> +    touch $GIT_DIR/objects/info/alternates\n>> +    echo \"$(readlink -f $dir)\" >> $GIT_DIR/objects/info/alternates\n>\n> What is this touch for?\n\nI wasn't 100% sure >> would work if the file didn't exist. I just\ntried on a bash shell and it works. Does anyone know of any supported\nshells that behave differently w.r.t the >> operator?\n\n> Is readlink(1) portable enough across platforms?  A more fundamental\n> question is if resolving symbolic link like this behind user is a sensible\n> thing to do, especially as you are ...\n>\n>> +    say \"$dir added as an alternate\"\n>\n> ... lying to the user here, if $dir indeed is a symbolic link.\n\nUsing readlink was my hack around converting a user specified relative\npath to an absolute one. I actually would prefer it if it didn't\ninterpret a symlink. I also had my doubts about portability its\nprobably not going to exist on platforms that don't have real symlinks\n(windows). What I really wanted to do was something like \"abspath\n$dir\" but I was bitterly disappointed to find that was a gnu make-ism.\nAny suggestions?\n\n>> +rewrite_alternates()\n>> +{\n>> +    if test \"$1\" != \"$2\"; then\n>> +        echo $2 >> $3\n>> +    fi\n>> +}\n>\n> That's a misleading name for this helper function.\n\nI think this is made redundant by your grep suggestion below.\n\n>> +del_alternate()\n>> +{\n>> +    if test ! $force = \"true\"; then\n>> +        say \"Not forced, use\"\n>> +        say \"   'git repack -a' to fetch missing objects, then \"\n>> +        say \"   '$dashless -f -d $dir' to remove the alternate\"\n>> +        die\n>> +    fi\n>> +\n>> +    local alternates=$GIT_DIR/objects/info/alternates\n>> +\n>> +    new_alts_file=$(mktemp $alternates-XXXXXX)\n>> +    touch $new_alts_file\n>> +\n>> +    walk_alternates rewrite_alternates $dir $new_alts_file\n>\n> I think this is a more expensive way to say:\n>\n>        grep -v -F \"$dir\" <\"$alternates\" >\"$new_alternates\"\n>\n\nMuch easier. I'll do that.\n\n>> +    mv $new_alts_file $alternates\n>> +\n>> +    # save the git from repeatedly reading a 0 length file\n>> +    if test $(stat -c \"%s\" $alternates) -eq 0; then\n>> +        rm $alternates\n>> +    fi\n>\n> Good point, but it would be better to do something like this:\n>\n>        grep -v -F \"$dir\" <\"$alternates\" >\"$new_alternates\"\n>        if test -s \"$new_alternates\"\n>        then\n>                mv \"$new_alternates\" \"$alternates\"\n>        else\n>                rm -f \"$alternates\"\n>        fi\n>\n> without making it less portable by using \"stat\".\n>\nYes. Will change.\n"},{"id":"137742","messageId":"a038bef51003241337p61705a35kd5ab6943bb27de36@mail.gmail.com","threadId":"23143","inReplyTo":"780e0a6b1003241316i4bbd4489w63ba0308706e4d20@mail.gmail.com","subject":"Re: Question about .git/objects/info/alternates","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2010-03-24T20:37:47Z","receivedAt":"2010-03-24T20:37:47Z","isPatch":false,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"On Wed, Mar 24, 2010 at 1:16 PM, Stephen Boyd <bebarino@gmail.com> wrote:\n> On Wed, Mar 24, 2010 at 11:53 AM, Chris Packham <judge.packham@gmail.com> wrote:\n>>\n>> +USAGE='[-r|--recursive] [-a|--add <dir>] [-f|--force -d|--delete <dir>]'\n> [...]\n>> +case $oper in\n>> +    add) add_alternate ;;\n>> +    del) del_alternate ;;\n>> +    *)   show_alternates ;;\n>> +esac\n>\n> From a very high-level this should probably be more like git remote\n> and git notes. 'add' and 'delete' would be subcommands instead of\n> options. Plus you might have an explicit subcommand for show (or\n> list?). Something like\n>\n> git alternates [show]  [-r|--recursive]\n> git alternates add <dir>\n> git alternates delete  [-f|--force] <dir>\n>\n\nI like that suggestion. I'd have to figure out the option parsing but\nshould be doable. This is of course assuming it remains its own\ncommand set and isn't rolled into something else.\n"},{"id":"137781","messageId":"1269497251-13103-1-git-send-email-judge.packham@gmail.com","threadId":"23143","inReplyTo":"a038bef51003241335l1623ade4i4a9b7269546739d4@mail.gmail.com","subject":"Re: Question about .git/objects/info/alternates","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2010-03-25T06:07:29Z","receivedAt":"2010-03-25T06:07:29Z","isPatch":false,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"Heres my updated patch. I think I've addressed all the comments except\nStephen's.\n\nI've also added some tests in the 2nd patch but I'm running into a problem\nwhere all the tests pass but the overall test fails:\n  FATAL: Unexpected exit with code 0\n  make: *** [t9800-git-alternate.sh] Error 1\n\nMy current TODO list is\n- fix the test\n- add tests for git alternate --delete\n- add documentation\n- refine the display output, its a bit jumbled at the moment it could be better\n\nI'm travelling for the next couple of weeks so I may not be that quick to\nrespond to comments.\n"},{"id":"137782","messageId":"1269497251-13103-2-git-send-email-judge.packham@gmail.com","threadId":"23143","inReplyTo":"a038bef51003241335l1623ade4i4a9b7269546739d4@mail.gmail.com","subject":"[PATCHv2 1/2] Add git alternate command","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2010-03-25T06:07:30Z","receivedAt":"2010-03-25T06:07:30Z","isPatch":false,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"The current UI around alternates is lacking. There are commands to add\nan alternate at the time of a clone (e.g. 'git clone --reference') but\nno commands to see what alternates have been configured or to add an\nalternate after a repository has been cloned. This patch adds a\nfriendlier UI for displaying and configuring alternates.\n\nSigned-off-by: Chris Packham <judge.packham@gmail.com>\n---\nHopefully I've addressed the comments from Junio and Jonathan. I haven't tried\nto re-work the commands into show|add|delete as per Stephens suggestion. I have\nsubtly changed from 'alternates' to 'alternate' i.e. ditched the plural in line\nwith commands like 'git remote'\n\n Makefile         |    1 +\n git-alternate.sh |  155 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 156 insertions(+), 0 deletions(-)\n create mode 100755 git-alternate.sh\n\ndiff --git a/Makefile b/Makefile\nindex 3a6c6ea..0dc5e42 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -334,6 +334,7 @@ TEST_PROGRAMS_NEED_X =\n unexport CDPATH\n \n SCRIPT_SH += git-am.sh\n+SCRIPT_SH += git-alternate.sh\n SCRIPT_SH += git-bisect.sh\n SCRIPT_SH += git-difftool--helper.sh\n SCRIPT_SH += git-filter-branch.sh\ndiff --git a/git-alternate.sh b/git-alternate.sh\nnew file mode 100755\nindex 0000000..827baf5\n--- /dev/null\n+++ b/git-alternate.sh\n@@ -0,0 +1,155 @@\n+#!/bin/sh\n+#\n+# This file is licensed under the GPL v2\n+#\n+\n+OPTIONS_KEEPDASHDASH=\n+OPTIONS_SPEC=\"\\\n+git alternate [-r|--recursive]\n+git alternate [-a|--add dir]\n+git alternate [-f|--force] [-d|delete dir]\n+--\n+r,recursive                    recursively follow alternates\n+a,add=                         add dir as an alternate\n+d,del=                         delete dir as an alternate\n+f,force                        force a delete operation\n+\"\n+. git-sh-setup\n+\n+#\n+# Given the path in $1, which may or may not be a relative path,\n+# convert it to an absolute path\n+#\n+abspath()\n+{\n+\tcd \"$1\"\n+\tpwd\n+\tcd - > /dev/null\n+}\n+\n+#\n+# Runs through the alternates file calling the callback function $1\n+# with the name of the alternate as the first argument to the callback\n+# any additional arguments are passed to the callback function.\n+#\n+walk_alternates()\n+{\n+\talternates=$GIT_DIR/objects/info/alternates\n+\tcallback=$1\n+\tshift\n+\n+\tif test -f \"$alternates\"\n+\tthen\n+\t\twhile read line\n+\t\tdo\n+\t\t\t$callback \"$line\" \"$@\"\n+\t\tdone< \"$alternates\"\n+\tfi\n+}\n+\n+#\n+# Walk function to display one alternate object store and, if the user\n+# has specified -r, recursively call show_alternates on the git \n+# repository that the object store belongs to.\n+#\n+show_alternates_walk()\n+{\n+\tsay \"Object store $1\"\n+\tsay \"    referenced via $GIT_DIR\"\n+\n+\tnew_git_dir=${line%%/objects}\n+\tif test \"$recursive\" = \"true\" && test \"$GIT_DIR\" != \"$new_git_dir\"\n+\tthen\n+\t(\n+\t\texport GIT_DIR=$new_git_dir\n+\t\tshow_alternates\n+\t)\n+\tfi\n+}\n+\n+# Display alternates currently configured\n+show_alternates()\n+{\n+\twalk_alternates show_alternates_walk\n+}\n+\n+#\n+# Walk function to check that the specified alternate does not\n+# already exist.\n+#\n+check_current_alternate_walk()\n+{\n+\tif test \"$1\" = \"$2\"; then\n+\t\tdie \"fatal: Object store $2 is already used by $GIT_DIR\"\n+\tfi\n+}\n+\n+# Add a new alternate\n+add_alternate()\n+{\n+\tif test ! -d \"$dir\"; then\n+\t\tdie \"fatal: $dir is not a directory\"\n+\tfi\n+\n+\tabs_dir=$(abspath \"$dir\")\n+\twalk_alternates check_current_alternate_walk \"$abs_dir\"\n+\n+\t# At this point we know that $dir is a directory that exists\n+\t# and that its not already being used as an alternate. We could\n+\t# go further and verify that $dir has valid objects.\n+\n+\t# if we're still going we can safely add the alternate\n+\techo \"$abs_dir\" >> $GIT_DIR/objects/info/alternates\n+\tsay \"$abs_dir added as an alternate\"\n+\tsay \"    use 'git repack -adl' to remove duplicate objects\"\n+}\n+\n+# Deletes the name alternate from the alternates file.\n+# If there are no more alternates the alternates file will be removed\n+del_alternate()\n+{\n+\tif test ! $force = \"true\"; then\n+\t\tsay \"Not forced, use\"\n+\t\tsay \"    'git repack -a' to fetch missing objects, then \"\n+\t\tsay \"    '$dashless -f -d $dir' to remove the alternate\"\n+\t\tdie\n+\tfi\n+\n+\talternates=$GIT_DIR/objects/info/alternates\n+\tnew_alternates=$alternates.tmp\n+\n+\tgrep -v -F \"$dir\" <\"$alternates\" >\"$new_alternates\"\n+\tif test -s \"$new_alternates\"\n+\tthen\n+\t\tmv \"$new_alternates\" \"$alternates\"\n+\telse\n+\t\t# save the git from repeatedly reading a 0 length file\n+\t\trm -f \"$alternates\"\n+\tfi\n+}\n+\n+dir=\"\"\n+oper=\"\"\n+force=\"false\"\n+\n+# Option parsing\n+while test $# != 0\n+do\n+\tcase \"$1\" in\n+\t-r|--recursive)  recursive=\"true\" ;;\n+\t-a|--add)        oper=\"add\"; dir=\"$2\"; shift ;;\n+\t-d|--delete)     oper=\"del\"; dir=\"$2\"; shift ;;\n+\t-f|--force)      force=\"true\" ;;\n+\t--)              shift; break ;;\n+\t*)               usage ;;\n+\tesac\n+\tshift\n+done\n+\n+# Now go and do it\n+case \"$oper\" in\n+\tadd) add_alternate ;;\n+\tdel) del_alternate ;;\n+\t*)   show_alternates ;;\n+esac\n+\n-- \n1.7.0.3\n"},{"id":"137783","messageId":"1269497251-13103-3-git-send-email-judge.packham@gmail.com","threadId":"23143","inReplyTo":"a038bef51003241335l1623ade4i4a9b7269546739d4@mail.gmail.com","subject":"[PATCHv2 2/2] tests for git alternate command","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2010-03-25T06:07:31Z","receivedAt":"2010-03-25T06:07:31Z","isPatch":false,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"Signed-off-by: Chris Packham <judge.packham@gmail.com>\n---\nI wasn't sure about the test numbering so I just grabbed the highest one. Still\nneed to add tests for the deletion use case.\n\n t/t9800-git-alternate.sh |   95 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 95 insertions(+), 0 deletions(-)\n create mode 100644 t/t9800-git-alternate.sh\n\ndiff --git a/t/t9800-git-alternate.sh b/t/t9800-git-alternate.sh\nnew file mode 100644\nindex 0000000..e95f5bd\n--- /dev/null\n+++ b/t/t9800-git-alternate.sh\n@@ -0,0 +1,95 @@\n+#!/bin/sh\n+\n+test_description='Test of git alternate command'\n+\n+. ./test-lib.sh\n+\n+test_expect_success \\\n+\t'Setup for rest of the test' '\n+\tmkdir -p base &&\n+\tcd base &&\n+\tgit init &&\n+\techo test > a.txt &&\n+\techo test > b.txt &&\n+\techo test > c.txt &&\n+\tgit add *.txt &&\n+\tgit commit -a -m \"Initial Commit\" &&\n+\tcd .. &&\n+\tgit clone base A &&\n+\tgit clone A B &&\n+\tgit clone A C &&\n+\tgit clone A D\n+'\n+\n+test_expect_success \\\n+\t'Add alternate after clone' '\n+\t(cd B &&\n+\tgit alternate -a ../base/.git/objects\n+\t)\n+'\n+\n+test_expect_success \\\n+\t'add same alternate fails adding existing abs path' '\n+\t(cd B &&\n+\ttest_must_fail git alternate -a $PWD/base/.git/objects\n+\t)\n+'\n+\n+test_expect_success \\\n+ \t'add same alternate fails adding existing relative path' '\n+ \t(cd B &&\n+ \ttest_must_fail git alternate -a ../base/.git/objects\n+ \t)\n+'\n+\n+test_expect_success \\\n+\t'Add multiple alternates' '\n+\t(cd C &&\n+\tgit alternate -a ../base/.git/objects &&\n+\tgit alternate -a ../B/.git/objects\n+\t)\n+'\n+\n+test_expect_success \\\n+\t'Add recursive alternate' '\n+\t(cd D &&\n+\tgit alternate -a ../C/.git/objects\n+\t)\n+'\n+\n+test_expect_success \\\n+\t'test git alternate display' '\n+\ttestbase=$PWD\n+\t(cd B &&\n+\tgit alternate >actual &&\n+\t{\n+\t\techo \"Object store $testbase/base/.git/objects\"\n+\t\techo \"    referenced via $testbase/B/.git\"\n+\t} >expect &&\n+\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success \\\n+\t'test git alternate recursive display' '\n+\ttestbase=$PWD\n+\t(cd D &&\n+\n+\tgit alternate -r >actual &&\n+\n+\t{\n+\t\techo \"Object store $testbase/C/.git/objects\"\n+\t\techo \"    referenced via $testbase/D/.git\"\n+\t\techo \"Object store $testbase/base/.git/objects\"\n+\t\techo \"    referenced via $testbase/C/.git\"\n+\t\techo \"Object store $testbase/B/.git/objects\"\n+\t\techo \"    referenced via $testbase/C/.git\"\n+\t\techo \"Object store $testbase/base/.git/objects\"\n+\t\techo \"    referenced via $testbase/B/.git\"\n+\t} >expect &&\n+\n+\ttest_cmp expect actual\n+\t)\n+'\n+\n+#rm -rf A B C D base\n\\ No newline at end of file\n-- \n1.7.0.3\n"},{"id":"137785","messageId":"4BAB12E1.8070602@viscovery.net","threadId":"23143","inReplyTo":"1269497251-13103-3-git-send-email-judge.packham@gmail.com","subject":"Re: [PATCHv2 2/2] tests for git alternate command","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-03-25T07:38:09Z","receivedAt":"2010-03-25T07:38:09Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/25/2010 7:07, schrieb Chris Packham:\n> Signed-off-by: Chris Packham <judge.packham@gmail.com>\n\nYour patches violate whitespace rules. Use 'git show --check' to see the\nquestionable lines.\n\n> ---\n> I wasn't sure about the test numbering so I just grabbed the highest one. Still\n> need to add tests for the deletion use case.\n\nAccording to t/README, t1* would be a suitable category, perhaps t1430.\n\n>  t/t9800-git-alternate.sh |   95 ++++++++++++++++++++++++++++++++++++++++++++++\n\n\"git\" in the name is redundant\n\n> +test_expect_success \\\n> +\t'Setup for rest of the test' '\n\nThe modern style for test headers is\n\ntest_expect_success 'Setup for rest of the test' '\n\ttest goes here\n'\n\nIt may improve readability if you insert a blank line after the headline.\n\n> +\tmkdir -p base &&\n> +\tcd base &&\n> +\tgit init &&\n> +\techo test > a.txt &&\n> +\techo test > b.txt &&\n> +\techo test > c.txt &&\n> +\tgit add *.txt &&\n> +\tgit commit -a -m \"Initial Commit\" &&\n> +\tcd .. &&\n\nDo not use 'cd dir && ... && cd ..', use (cd dir && ...) like you did in\nthe rest of the tests.\n\n> +test_expect_success \\\n> +\t'Add alternate after clone' '\n> +\t(cd B &&\n> +\tgit alternate -a ../base/.git/objects\n> +\t)\n\nWe saw tests like this written with more whitespace like:\n\n\t(\n\t\tcd B &&\n\t\tgit alternate -a ../base/.git/objects\n\t)\n\n> +test_expect_success \\\n> +\t'add same alternate fails adding existing abs path' '\n> +\t(cd B &&\n> +\ttest_must_fail git alternate -a $PWD/base/.git/objects\n\nThis use of $PWD is OK, but for consistency it should be $(pwd) like\nbelow. Moreover, it needs double-quotes.\n\n> +test_expect_success \\\n> +\t'test git alternate display' '\n> +\ttestbase=$PWD\n\nYou must write this as (d-quotes not needed here)\n\n\ttestbase=$(pwd) &&\n\nfor the benefit of Windows. The difference is that $PWD returns /c/path,\nbut $(pwd) returns c:/path.\n\nWhen the alternate was set up using the command, git has only ever seen a\nc:/path style path (regardless of whether you used $PWD or $(pwd), because\nthe path is converted to c:/path by the shell before it invokes git), and\ntherefore the alternates file contains this style.\n\nBut when the expected result is constructed, the /c/path style is *not*\nconverted to c:/path; and a mismatch would be detected.\n\n> +\t(cd B &&\n> +\tgit alternate >actual &&\n> +\t{\n> +\t\techo \"Object store $testbase/base/.git/objects\"\n> +\t\techo \"    referenced via $testbase/B/.git\"\n> +\t} >expect &&\n> +\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n> +test_expect_success \\\n> +\t'test git alternate recursive display' '\n> +\ttestbase=$PWD\n\nDitto.\n\n> +#rm -rf A B C D base\n> \\ No newline at end of file\n\ntest_done\n\n-- Hannes\n"},{"id":"137807","messageId":"a038bef51003251151h5ede3ab8jc22a61cc35706240@mail.gmail.com","threadId":"23143","inReplyTo":"4BAB12E1.8070602@viscovery.net","subject":"Re: [PATCHv2 2/2] tests for git alternate command","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2010-03-25T18:51:15Z","receivedAt":"2010-03-25T18:51:15Z","isPatch":false,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"On Thu, Mar 25, 2010 at 12:38 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> Am 3/25/2010 7:07, schrieb Chris Packham:\n>> Signed-off-by: Chris Packham <judge.packham@gmail.com>\n>\n> Your patches violate whitespace rules. Use 'git show --check' to see the\n> questionable lines.\n>\n\nWill do. BTW theres no mention of --check in git show --help, but one\nitch at a time.\n\n>> ---\n>> I wasn't sure about the test numbering so I just grabbed the highest one. Still\n>> need to add tests for the deletion use case.\n>\n> According to t/README, t1* would be a suitable category, perhaps t1430.\n>\n>>  t/t9800-git-alternate.sh |   95 ++++++++++++++++++++++++++++++++++++++++++++++\n>\n> \"git\" in the name is redundant\n>\n\nIs t1430-alternate-cmd.sh OK with everyone?\n\n>> +test_expect_success \\\n>> +     'Setup for rest of the test' '\n>\n> The modern style for test headers is\n>\n\n[snip]\n\nWill incorporate in next update\n\n>> +#rm -rf A B C D base\n>> \\ No newline at end of file\n>\n> test_done\n>\n\nAh, I knew it would be something simple like that. I guess I should\nhave started by reading t/README.\n"},{"id":"137820","messageId":"20100326004845.GS27414@genesis.frugalware.org","threadId":"23143","inReplyTo":"a038bef51003251151h5ede3ab8jc22a61cc35706240@mail.gmail.com","subject":"Re: [PATCHv2 2/2] tests for git alternate command","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2010-03-26T00:48:45Z","receivedAt":"2010-03-26T00:48:45Z","isPatch":false,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Thu, Mar 25, 2010 at 11:51:15AM -0700, Chris Packham <judge.packham@gmail.com> wrote:\n> On Thu, Mar 25, 2010 at 12:38 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> > Am 3/25/2010 7:07, schrieb Chris Packham:\n> >> Signed-off-by: Chris Packham <judge.packham@gmail.com>\n> >\n> > Your patches violate whitespace rules. Use 'git show --check' to see the\n> > questionable lines.\n> >\n> \n> Will do. BTW theres no mention of --check in git show --help, but one\n> itch at a time.\n\nSee http://thread.gmane.org/gmane.comp.version-control.git/80442\n"},{"id":"137830","messageId":"4BAC57CD.2070200@viscovery.net","threadId":"23143","inReplyTo":"a038bef51003251151h5ede3ab8jc22a61cc35706240@mail.gmail.com","subject":"Re: [PATCHv2 2/2] tests for git alternate command","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-03-26T06:44:29Z","receivedAt":"2010-03-26T06:44:29Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/25/2010 19:51, schrieb Chris Packham:\n> Is t1430-alternate-cmd.sh OK with everyone?\n\nIt's fine.\n\n-- Hannes\n"},{"id":"138061","messageId":"7vr5n37fci.fsf@alter.siamese.dyndns.org","threadId":"23143","inReplyTo":"1269497251-13103-2-git-send-email-judge.packham@gmail.com","subject":"Re: [PATCHv2 1/2] Add git alternate command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-03-29T07:32:29Z","receivedAt":"2010-03-29T07:32:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Packham <judge.packham@gmail.com> writes:\n\n> +abspath()\n> +{\n> +\tcd \"$1\"\n> +\tpwd\n> +\tcd - > /dev/null\n> +}\n\nI do not think \"cd -\" is all that portable.  As you will be always using this in\nthis form:\n\n\tsomevariable=$(abspath \"$it\")\n\nyou are in a subshell and won't affect the caller anyway.  Why not drop\nthe \"go back to where we came from\"?\n\nAlso a shell built-in \"pwd\" tends to be fooled by $PWD especially since\nyou are running \"cd\" without -P.\n\n> +#\n> +# Runs through the alternates file calling the callback function $1\n> +# with the name of the alternate as the first argument to the callback\n> +# any additional arguments are passed to the callback function.\n> +#\n> +walk_alternates()\n\nThis is more like \"for_each_alternates\"; \"walk\" can be mistaken as if you\nare recursively looking at alternates defined in alternate repositories\nof the repository you start from.\n\n> +{\n> +\talternates=$GIT_DIR/objects/info/alternates\n> +\tcallback=$1\n> +\tshift\n> +\n> +\tif test -f \"$alternates\"\n> +\tthen\n> +\t\twhile read line\n> +\t\tdo\n> +\t\t\t$callback \"$line\" \"$@\"\n> +\t\tdone< \"$alternates\"\n\n\tdone <\"$alternates\"\n\nHow well does this handle relative alternate object stores?  Shouldn't it\nbe more like this?\n\n\twhile read altdir\n        do\n        \tcase \"$altdir\" in\n                /*) ;; # full path\n                *) altdir=\"$GIT_DIR/objects/$altdir\" ;;\n                esac &&\n                $callback \"$altdir\" \"$@\"\n\tdone <\"$alternates\"\n\n> +# Walk function to display one alternate object store and, if the user\n> +# has specified -r, recursively call show_alternates on the git \n> +# repository that the object store belongs to.\n> +#\n> +show_alternates_walk()\n> +{\n> +\tsay \"Object store $1\"\n> +\tsay \"    referenced via $GIT_DIR\"\n> +\n> +\tnew_git_dir=${line%%/objects}\n\nDo you need double-% here, not a single one?\n\n> +# Add a new alternate\n> +add_alternate()\n> +{\n> +\tif test ! -d \"$dir\"; then\n> +\t\tdie \"fatal: $dir is not a directory\"\n> +\tfi\n> +\n> +\tabs_dir=$(abspath \"$dir\")\n> +\twalk_alternates check_current_alternate_walk \"$abs_dir\"\n> +\n> +\t# At this point we know that $dir is a directory that exists\n> +\t# and that its not already being used as an alternate. We could\n\ns/its/(it's|it is)/;\n\nBut I don't think it is true that you have verified that it is not used.\nYou are running abspath on the input from the end user, but you are using\nexisting entries in the alternates file that may not be absolute.  They\ncan be relative to $GIT_DIR/objects/.\n\n> +\tsay \"    use 'git repack -adl' to remove duplicate objects\"\n\nGood.\n\n> +# Deletes the name alternate from the alternates file.\n> +# If there are no more alternates the alternates file will be removed\n> +del_alternate()\n> +{\n> +\tif test ! $force = \"true\"; then\n> +\t\tsay \"Not forced, use\"\n> +\t\tsay \"    'git repack -a' to fetch missing objects, then \"\n> +\t\tsay \"    '$dashless -f -d $dir' to remove the alternate\"\n> +\t\tdie\n\nHmm, I am afraid that this will end up training users to always say -f\nwithout even thinking.  Shouldn't this code be doing whatever necessary\nsteps to make sure this repository has all the necessary objects without\nthe named alternates and then removing the file?  An easiest might be to\ntemporarily remove the entry and run fsck, perhaps?\n"},{"id":"138259","messageId":"a038bef51003302135k700446n5a5e9d50e35d1ad9@mail.gmail.com","threadId":"23143","inReplyTo":"7vr5n37fci.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 1/2] Add git alternate command","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2010-03-31T04:35:16Z","receivedAt":"2010-03-31T04:35:16Z","isPatch":false,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"Sorry, just realized I missed this email before sending my last update.\n\nOn Mon, Mar 29, 2010 at 12:32 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Chris Packham <judge.packham@gmail.com> writes:\n>\n>> +abspath()\n>> +{\n>> +     cd \"$1\"\n>> +     pwd\n>> +     cd - > /dev/null\n>> +}\n>\n> I do not think \"cd -\" is all that portable.  As you will be always using this in\n> this form:\n>\n>        somevariable=$(abspath \"$it\")\n>\n> you are in a subshell and won't affect the caller anyway.  Why not drop\n> the \"go back to where we came from\"?\n\nFair enough. Should be safe.\n\n> Also a shell built-in \"pwd\" tends to be fooled by $PWD especially since\n> you are running \"cd\" without -P.\n\nAre you saying I should switch to \"echo $PWD\"?\n\n>> +#\n>> +# Runs through the alternates file calling the callback function $1\n>> +# with the name of the alternate as the first argument to the callback\n>> +# any additional arguments are passed to the callback function.\n>> +#\n>> +walk_alternates()\n>\n> This is more like \"for_each_alternates\"; \"walk\" can be mistaken as if you\n> are recursively looking at alternates defined in alternate repositories\n> of the repository you start from.\n\nEasy change.\n\n>> +{\n>> +     alternates=$GIT_DIR/objects/info/alternates\n>> +     callback=$1\n>> +     shift\n>> +\n>> +     if test -f \"$alternates\"\n>> +     then\n>> +             while read line\n>> +             do\n>> +                     $callback \"$line\" \"$@\"\n>> +             done< \"$alternates\"\n>\n>        done <\"$alternates\"\n>\n> How well does this handle relative alternate object stores?\n\nNot well at all.\n\n> Shouldn't it be more like this?\n>\n>        while read altdir\n>        do\n>                case \"$altdir\" in\n>                /*) ;; # full path\n>                *) altdir=\"$GIT_DIR/objects/$altdir\" ;;\n>                esac &&\n>                $callback \"$altdir\" \"$@\"\n>        done <\"$alternates\"\n>\n\nThanks, I was wondering how to handle relative paths.\n\n>> +# Walk function to display one alternate object store and, if the user\n>> +# has specified -r, recursively call show_alternates on the git\n>> +# repository that the object store belongs to.\n>> +#\n>> +show_alternates_walk()\n>> +{\n>> +     say \"Object store $1\"\n>> +     say \"    referenced via $GIT_DIR\"\n>> +\n>> +     new_git_dir=${line%%/objects}\n>\n> Do you need double-% here, not a single one?\n>\n\nNot if I'm not using a regex. I'll change it.\n\n>> +# Add a new alternate\n>> +add_alternate()\n>> +{\n>> +     if test ! -d \"$dir\"; then\n>> +             die \"fatal: $dir is not a directory\"\n>> +     fi\n>> +\n>> +     abs_dir=$(abspath \"$dir\")\n>> +     walk_alternates check_current_alternate_walk \"$abs_dir\"\n>> +\n>> +     # At this point we know that $dir is a directory that exists\n>> +     # and that its not already being used as an alternate. We could\n>\n> s/its/(it's|it is)/;\n>\n> But I don't think it is true that you have verified that it is not used.\n> You are running abspath on the input from the end user, but you are using\n> existing entries in the alternates file that may not be absolute.  They\n> can be relative to $GIT_DIR/objects/.\n>\n\nI'll add some actual verification\n\n>> +     say \"    use 'git repack -adl' to remove duplicate objects\"\n>\n> Good.\n>\n>> +# Deletes the name alternate from the alternates file.\n>> +# If there are no more alternates the alternates file will be removed\n>> +del_alternate()\n>> +{\n>> +     if test ! $force = \"true\"; then\n>> +             say \"Not forced, use\"\n>> +             say \"    'git repack -a' to fetch missing objects, then \"\n>> +             say \"    '$dashless -f -d $dir' to remove the alternate\"\n>> +             die\n>\n> Hmm, I am afraid that this will end up training users to always say -f\n> without even thinking.  Shouldn't this code be doing whatever necessary\n> steps to make sure this repository has all the necessary objects without\n> the named alternates and then removing the file?  An easiest might be to\n> temporarily remove the entry and run fsck, perhaps?\n>\n\nYeah I had the same fear. I was wondering how to proceed with this part.\n\nI take it your suggestion is to update the alternates file and run git\nfsck to see if any objects are missing then, if there are missing\nobjects, switch to the original alternates file run git repack -a to\nfetch the missing objects and finally switch to the new alternates\nfile. I'll code something up for us to discuss.\n"}]}