* Question about .git/objects/info/alternates
@ 2010-03-22 17:26 Chris Packham
2010-03-23 2:42 ` Jonathan Nieder
0 siblings, 1 reply; 17+ messages in thread
From: Chris Packham @ 2010-03-22 17:26 UTC (permalink / raw)
To: GIT
Hi All,
I've been using git clone --reference and git submodule update
--reference to reduce the amount of data transferred when I cloned a
repository that I already had an older copy of (that for one reason or
another I didn't want to touch or clone directly).
Now I'm finding that what I really want to do is change around what is
referencing what. I currently have the following.
projecta.git
base.git # references project a
projectb.git # referenced base (which, now that I think about it,
was probably the wrong thing to do)
Ideally I'd want to end up with
base.git # has all objects
projecta.git # uses base as a reference
projectb.git # uses base as a reference also
I would like to have base somehow find the objects it doesn't have in
its object store and either download them or just copy them from the
object store of projecta. Then I can manually point projecta at base
and repack (as discussed in this thread [1]) to free up some space.
projectb should be fine as is because it already references base. Is
there any way to actually do this? A little googling found hints on
adding alternates after the fact but I'm actually interested in going
the other direction. From reading [2] I think 'rm
.git/objects/info/alternates && git repack -a' might do the trick but
I'm not sure.
[1] http://thread.gmane.org/gmane.comp.version-control.git/141161/focus=141199
[2] http://stackoverflow.com/questions/2248228/how-to-detach-alternates-after-git-clone-reference
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Question about .git/objects/info/alternates
2010-03-22 17:26 Question about .git/objects/info/alternates Chris Packham
@ 2010-03-23 2:42 ` Jonathan Nieder
2010-03-24 18:53 ` Chris Packham
0 siblings, 1 reply; 17+ messages in thread
From: Jonathan Nieder @ 2010-03-23 2:42 UTC (permalink / raw)
To: Chris Packham; +Cc: GIT
Chris Packham wrote:
> I would like to have base somehow find the objects it doesn't have in
> its object store and either download them or just copy them from the
> object store of projecta.
[...]
> From reading [2] I think 'rm
> .git/objects/info/alternates && git repack -a' might do the trick but
> I'm not sure.
Almost. Try ‘git repack -a && rm .git/objects/info/alternates’ instead. :)
(Please back up the repository or try with something less important
first, since I am not sure.)
Hope that helps,
Jonathan
> [2] http://stackoverflow.com/questions/2248228/how-to-detach-alternates-after-git-clone-reference
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Question about .git/objects/info/alternates
2010-03-23 2:42 ` Jonathan Nieder
@ 2010-03-24 18:53 ` Chris Packham
2010-03-24 19:23 ` Jonathan Nieder
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Chris Packham @ 2010-03-24 18:53 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: GIT
On Mon, Mar 22, 2010 at 7:42 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Chris Packham wrote:
>
>> I would like to have base somehow find the objects it doesn't have in
>> its object store and either download them or just copy them from the
>> object store of projecta.
> [...]
>> From reading [2] I think 'rm
>> .git/objects/info/alternates && git repack -a' might do the trick but
>> I'm not sure.
>
> Almost. Try ‘git repack -a && rm .git/objects/info/alternates’ instead. :)
>
> (Please back up the repository or try with something less important
> first, since I am not sure.)
>
> Hope that helps,
> Jonathan
>
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). So here's my attempt to add a 'git
alternates' command which can display, add or remove an alternate. The
adding and removing could be done with standard shell commands but
I've found the recursive displaying quite useful and couldn't think of
a simple command line to achieve the same thing .
------8<------
From a5c64de20937da132376d717f19a1d52b54701d2 Mon Sep 17 00:00:00 2001
From: Chris Packham <judge.packham@gmail.com>
Date: Wed, 24 Mar 2010 11:34:11 -0700
Subject: [PATCH] Add git alternates command
Provides a friendlier UI for displaying and configuring alternates.
Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
This patch assumes multiple alterates are possible. If this is not correct then
it could be simplfied as there would be no need for walk_alternates.
Makefile | 1 +
git-alternates.sh | 159 +++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 160 insertions(+), 0 deletions(-)
create mode 100755 git-alternates.sh
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
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
+#
+# This file is licensed under the GPL v2
+#
+
+USAGE='[-r|--recursive] [-a|--add <dir>] [-f|--force -d|--delete <dir>]'
+
+. git-sh-setup
+
+#
+# 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
+ shift
+
+ if [ -e $alternates ]; then
+ while read line
+ do
+ $callback $line $*
+ done < $alternates
+ fi
+}
+
+#
+# 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
+ GIT_DIR=$new_git_dir show_alternates
+ fi
+}
+
+show_alternates()
+{
+ walk_alternates show_alternates_walk
+}
+
+#
+# Walk function to check that the specified alternate does not
+# already exist.
+#
+check_current_alternate_walk()
+{
+ if test "$1" = "$2"; then
+ die "fatal: Object store $2 is already used by $GIT_DIR"
+ fi
+}
+
+add_alternate()
+{
+ if test ! -d $dir; then
+ die "fatal: $dir is not a directory"
+ fi
+
+ 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
+ say "$dir added as an alternate"
+ say " use 'git repack -adl' to remove duplicate objects"
+}
+
+rewrite_alternates()
+{
+ if test "$1" != "$2"; then
+ echo $2 >> $3
+ fi
+}
+
+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
+ 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
+}
+
+dir=""
+oper=""
+force="false"
+
+# Option parsing
+while test $# != 0
+do
+ case "$1" in
+ -r|--recursive)
+ recursive="true"
+ ;;
+ -a|--add)
+ if test ! -z "$oper"; then
+ usage
+ fi
+ oper="add"
+ case "$#,$1" in
+ 1,*) usage ;;
+ *) dir=$2; shift ;;
+ esac
+ ;;
+ -d|--delete)
+ if test ! -z "$oper"; then
+ usage
+ fi
+ oper="del"
+ case "$#,$1" in
+ 1,*) usage ;;
+ *) dir=$2; shift ;;
+ esac
+ ;;
+ -f|--force)
+ force="true"
+ ;;
+ -*)
+ usage
+ ;;
+ *)
+ ;;
+ esac
+ shift
+done
+
+# Now go and do it
+case $oper in
+ add) add_alternate ;;
+ del) del_alternate ;;
+ *) show_alternates ;;
+esac
--
1.7.0.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: Question about .git/objects/info/alternates
2010-03-24 18:53 ` Chris Packham
@ 2010-03-24 19:23 ` Jonathan Nieder
2010-03-24 19:58 ` Junio C Hamano
2010-03-24 20:16 ` Question about .git/objects/info/alternates Stephen Boyd
2 siblings, 0 replies; 17+ messages in thread
From: Jonathan Nieder @ 2010-03-24 19:23 UTC (permalink / raw)
To: Chris Packham; +Cc: GIT
Chris Packham wrote:
> 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). So here's my attempt to add a 'git
> alternates' command which can display, add or remove an alternate.
Quite welcome!
Something like this explanation probably belongs in the commit
message. That way, years down the line, people don’t need to trawl
the list archives to see what your goal was.
Comments:
> From a5c64de20937da132376d717f19a1d52b54701d2 Mon Sep 17 00:00:00 2001
> From: Chris Packham <judge.packham@gmail.com>
> Date: Wed, 24 Mar 2010 11:34:11 -0700
Redundant next to the email header. It is useful to be able to include
fields that differ from the e-mail header (often Subject:,
sometimes Date:, sometimes From:), but aside from that, this metadata
should be omitted when sending patches to the git mailing list.
> +#
> +# 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
I couldn’t find any other uses of “local” in-tree. I assume old shells
don’t support it.
Will this be a problem for recursion? Maybe the callback should be
called in a subshell.
> + shift
> +
> + if [ -e $alternates ]; then
> + while read line
> + do
> + $callback $line $*
Probably "$line" "$@" instead of $line $* would be more flexible.
> +#
> +# 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}
use of local.
> + if [ "$recursive" == "true" -a "$GIT_DIR" != "$new_git_dir" ]
Should use = instead of == (portability). Also git scripts tend to
spell out 'test' and avoid the -a and -o operators:
if test "$recursive" = true && test "$GIT_DIR" != "$new_git-dir"
though that is not a hard and fast rule.
> +add_alternate()
> +{
[...]
> + touch $GIT_DIR/objects/info/alternates
Necessary?
> + echo "$(readlink -f $dir)" >> $GIT_DIR/objects/info/alternates
Maybe
readlink -f "$dir" >> $GIT_DIR/objects/info/alternates
would be simpler.
> +rewrite_alternates()
> +{
> + if test "$1" != "$2"; then
> + echo $2 >> $3
> + fi
> +}
What does this function do? (Could use a comment.)
> +del_alternate()
> +{
[...]
> + local alternates=$GIT_DIR/objects/info/alternates
use of local.
> +
> + new_alts_file=$(mktemp $alternates-XXXXXX)
Not used elsewhere in git. Is this needed? Maybe a single
$GIT_DIR/objects/info/new-alternates.tmp or similar would be good
enough.
> + # save the git from repeatedly reading a 0 length file
> + if test $(stat -c "%s" $alternates) -eq 0; then
Not used elsewhere in core git. test -s can help.
> +# Option parsing
See OPTIONS_SPEC in git-repack.sh for an example of how to simplify
this.
> +# Now go and do it
> +case $oper in
> + add) add_alternate ;;
> + del) del_alternate ;;
> + *) show_alternates ;;
> +esac
Thank you for your excellent work! Looks very useful.
Regards,
Jonathan
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Question about .git/objects/info/alternates
2010-03-24 18:53 ` Chris Packham
2010-03-24 19:23 ` Jonathan Nieder
@ 2010-03-24 19:58 ` Junio C Hamano
2010-03-24 20:35 ` Chris Packham
2010-03-24 20:16 ` Question about .git/objects/info/alternates Stephen Boyd
2 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2010-03-24 19:58 UTC (permalink / raw)
To: Chris Packham; +Cc: Jonathan Nieder, GIT
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.
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.
> 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".
> 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
...
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.
> + 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.
> + 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)
> +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/.
> + 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?
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.
> +rewrite_alternates()
> +{
> + if test "$1" != "$2"; then
> + echo $2 >> $3
> + fi
> +}
That's a misleading name for this helper function.
> +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"
> + 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".
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Question about .git/objects/info/alternates
2010-03-24 18:53 ` Chris Packham
2010-03-24 19:23 ` Jonathan Nieder
2010-03-24 19:58 ` Junio C Hamano
@ 2010-03-24 20:16 ` Stephen Boyd
2010-03-24 20:37 ` Chris Packham
2 siblings, 1 reply; 17+ messages in thread
From: Stephen Boyd @ 2010-03-24 20:16 UTC (permalink / raw)
To: Chris Packham; +Cc: Jonathan Nieder, GIT
On Wed, Mar 24, 2010 at 11:53 AM, Chris Packham <judge.packham@gmail.com> wrote:
>
> +USAGE='[-r|--recursive] [-a|--add <dir>] [-f|--force -d|--delete <dir>]'
[...]
> +case $oper in
> + add) add_alternate ;;
> + del) del_alternate ;;
> + *) show_alternates ;;
> +esac
From a very high-level this should probably be more like git remote
and git notes. 'add' and 'delete' would be subcommands instead of
options. Plus you might have an explicit subcommand for show (or
list?). Something like
git alternates [show] [-r|--recursive]
git alternates add <dir>
git alternates delete [-f|--force] <dir>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Question about .git/objects/info/alternates
2010-03-24 19:58 ` Junio C Hamano
@ 2010-03-24 20:35 ` Chris Packham
2010-03-25 6:07 ` Chris Packham
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Chris Packham @ 2010-03-24 20:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, GIT
On Wed, Mar 24, 2010 at 12:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.
>> 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
>> 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.
>> + 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.
>> + 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.
>> +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.
>> + 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 ...
>
>> + 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?
>> +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.
>> +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.
>> + 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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Question about .git/objects/info/alternates
2010-03-24 20:16 ` Question about .git/objects/info/alternates Stephen Boyd
@ 2010-03-24 20:37 ` Chris Packham
0 siblings, 0 replies; 17+ messages in thread
From: Chris Packham @ 2010-03-24 20:37 UTC (permalink / raw)
To: Stephen Boyd; +Cc: Jonathan Nieder, GIT, Junio C Hamano
On Wed, Mar 24, 2010 at 1:16 PM, Stephen Boyd <bebarino@gmail.com> wrote:
> On Wed, Mar 24, 2010 at 11:53 AM, Chris Packham <judge.packham@gmail.com> wrote:
>>
>> +USAGE='[-r|--recursive] [-a|--add <dir>] [-f|--force -d|--delete <dir>]'
> [...]
>> +case $oper in
>> + add) add_alternate ;;
>> + del) del_alternate ;;
>> + *) show_alternates ;;
>> +esac
>
> From a very high-level this should probably be more like git remote
> and git notes. 'add' and 'delete' would be subcommands instead of
> options. Plus you might have an explicit subcommand for show (or
> list?). Something like
>
> git alternates [show] [-r|--recursive]
> git alternates add <dir>
> git alternates delete [-f|--force] <dir>
>
I like that suggestion. I'd have to figure out the option parsing but
should be doable. This is of course assuming it remains its own
command set and isn't rolled into something else.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Question about .git/objects/info/alternates
2010-03-24 20:35 ` Chris Packham
@ 2010-03-25 6:07 ` Chris Packham
2010-03-25 6:07 ` [PATCHv2 1/2] Add git alternate command Chris Packham
2010-03-25 6:07 ` [PATCHv2 2/2] tests for " Chris Packham
2 siblings, 0 replies; 17+ messages in thread
From: Chris Packham @ 2010-03-25 6:07 UTC (permalink / raw)
To: git; +Cc: jrnieder, gitster, bebarino
Heres my updated patch. I think I've addressed all the comments except
Stephen's.
I've also added some tests in the 2nd patch but I'm running into a problem
where all the tests pass but the overall test fails:
FATAL: Unexpected exit with code 0
make: *** [t9800-git-alternate.sh] Error 1
My current TODO list is
- fix the test
- add tests for git alternate --delete
- add documentation
- refine the display output, its a bit jumbled at the moment it could be better
I'm travelling for the next couple of weeks so I may not be that quick to
respond to comments.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCHv2 1/2] Add git alternate command
2010-03-24 20:35 ` Chris Packham
2010-03-25 6:07 ` Chris Packham
@ 2010-03-25 6:07 ` Chris Packham
2010-03-29 7:32 ` Junio C Hamano
2010-03-25 6:07 ` [PATCHv2 2/2] tests for " Chris Packham
2 siblings, 1 reply; 17+ messages in thread
From: Chris Packham @ 2010-03-25 6:07 UTC (permalink / raw)
To: git; +Cc: jrnieder, gitster, bebarino, Chris Packham
The current UI around alternates is lacking. There are commands to add
an alternate at the time of a clone (e.g. 'git clone --reference') but
no commands to see what alternates have been configured or to add an
alternate after a repository has been cloned. This patch adds a
friendlier UI for displaying and configuring alternates.
Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
Hopefully I've addressed the comments from Junio and Jonathan. I haven't tried
to re-work the commands into show|add|delete as per Stephens suggestion. I have
subtly changed from 'alternates' to 'alternate' i.e. ditched the plural in line
with commands like 'git remote'
Makefile | 1 +
git-alternate.sh | 155 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 156 insertions(+), 0 deletions(-)
create mode 100755 git-alternate.sh
diff --git a/Makefile b/Makefile
index 3a6c6ea..0dc5e42 100644
--- a/Makefile
+++ b/Makefile
@@ -334,6 +334,7 @@ TEST_PROGRAMS_NEED_X =
unexport CDPATH
SCRIPT_SH += git-am.sh
+SCRIPT_SH += git-alternate.sh
SCRIPT_SH += git-bisect.sh
SCRIPT_SH += git-difftool--helper.sh
SCRIPT_SH += git-filter-branch.sh
diff --git a/git-alternate.sh b/git-alternate.sh
new file mode 100755
index 0000000..827baf5
--- /dev/null
+++ b/git-alternate.sh
@@ -0,0 +1,155 @@
+#!/bin/sh
+#
+# This file is licensed under the GPL v2
+#
+
+OPTIONS_KEEPDASHDASH=
+OPTIONS_SPEC="\
+git alternate [-r|--recursive]
+git alternate [-a|--add dir]
+git alternate [-f|--force] [-d|delete dir]
+--
+r,recursive recursively follow alternates
+a,add= add dir as an alternate
+d,del= delete dir as an alternate
+f,force force a delete operation
+"
+. git-sh-setup
+
+#
+# Given the path in $1, which may or may not be a relative path,
+# convert it to an absolute path
+#
+abspath()
+{
+ cd "$1"
+ pwd
+ cd - > /dev/null
+}
+
+#
+# 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()
+{
+ alternates=$GIT_DIR/objects/info/alternates
+ callback=$1
+ shift
+
+ if test -f "$alternates"
+ then
+ while read line
+ do
+ $callback "$line" "$@"
+ done< "$alternates"
+ fi
+}
+
+#
+# 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"
+
+ new_git_dir=${line%%/objects}
+ if test "$recursive" = "true" && test "$GIT_DIR" != "$new_git_dir"
+ then
+ (
+ export GIT_DIR=$new_git_dir
+ show_alternates
+ )
+ fi
+}
+
+# Display alternates currently configured
+show_alternates()
+{
+ walk_alternates show_alternates_walk
+}
+
+#
+# Walk function to check that the specified alternate does not
+# already exist.
+#
+check_current_alternate_walk()
+{
+ if test "$1" = "$2"; then
+ die "fatal: Object store $2 is already used by $GIT_DIR"
+ fi
+}
+
+# Add a new alternate
+add_alternate()
+{
+ if test ! -d "$dir"; then
+ die "fatal: $dir is not a directory"
+ fi
+
+ abs_dir=$(abspath "$dir")
+ walk_alternates check_current_alternate_walk "$abs_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
+ echo "$abs_dir" >> $GIT_DIR/objects/info/alternates
+ say "$abs_dir added as an alternate"
+ say " use 'git repack -adl' to remove duplicate objects"
+}
+
+# Deletes the name alternate from the alternates file.
+# If there are no more alternates the alternates file will be removed
+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
+
+ alternates=$GIT_DIR/objects/info/alternates
+ new_alternates=$alternates.tmp
+
+ grep -v -F "$dir" <"$alternates" >"$new_alternates"
+ if test -s "$new_alternates"
+ then
+ mv "$new_alternates" "$alternates"
+ else
+ # save the git from repeatedly reading a 0 length file
+ rm -f "$alternates"
+ fi
+}
+
+dir=""
+oper=""
+force="false"
+
+# Option parsing
+while test $# != 0
+do
+ case "$1" in
+ -r|--recursive) recursive="true" ;;
+ -a|--add) oper="add"; dir="$2"; shift ;;
+ -d|--delete) oper="del"; dir="$2"; shift ;;
+ -f|--force) force="true" ;;
+ --) shift; break ;;
+ *) usage ;;
+ esac
+ shift
+done
+
+# Now go and do it
+case "$oper" in
+ add) add_alternate ;;
+ del) del_alternate ;;
+ *) show_alternates ;;
+esac
+
--
1.7.0.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCHv2 2/2] tests for git alternate command
2010-03-24 20:35 ` Chris Packham
2010-03-25 6:07 ` Chris Packham
2010-03-25 6:07 ` [PATCHv2 1/2] Add git alternate command Chris Packham
@ 2010-03-25 6:07 ` Chris Packham
2010-03-25 7:38 ` Johannes Sixt
2 siblings, 1 reply; 17+ messages in thread
From: Chris Packham @ 2010-03-25 6:07 UTC (permalink / raw)
To: git; +Cc: jrnieder, gitster, bebarino, Chris Packham
Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
I wasn't sure about the test numbering so I just grabbed the highest one. Still
need to add tests for the deletion use case.
t/t9800-git-alternate.sh | 95 ++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 95 insertions(+), 0 deletions(-)
create mode 100644 t/t9800-git-alternate.sh
diff --git a/t/t9800-git-alternate.sh b/t/t9800-git-alternate.sh
new file mode 100644
index 0000000..e95f5bd
--- /dev/null
+++ b/t/t9800-git-alternate.sh
@@ -0,0 +1,95 @@
+#!/bin/sh
+
+test_description='Test of git alternate command'
+
+. ./test-lib.sh
+
+test_expect_success \
+ 'Setup for rest of the test' '
+ mkdir -p base &&
+ cd base &&
+ git init &&
+ echo test > a.txt &&
+ echo test > b.txt &&
+ echo test > c.txt &&
+ git add *.txt &&
+ git commit -a -m "Initial Commit" &&
+ cd .. &&
+ git clone base A &&
+ git clone A B &&
+ git clone A C &&
+ git clone A D
+'
+
+test_expect_success \
+ 'Add alternate after clone' '
+ (cd B &&
+ git alternate -a ../base/.git/objects
+ )
+'
+
+test_expect_success \
+ 'add same alternate fails adding existing abs path' '
+ (cd B &&
+ test_must_fail git alternate -a $PWD/base/.git/objects
+ )
+'
+
+test_expect_success \
+ 'add same alternate fails adding existing relative path' '
+ (cd B &&
+ test_must_fail git alternate -a ../base/.git/objects
+ )
+'
+
+test_expect_success \
+ 'Add multiple alternates' '
+ (cd C &&
+ git alternate -a ../base/.git/objects &&
+ git alternate -a ../B/.git/objects
+ )
+'
+
+test_expect_success \
+ 'Add recursive alternate' '
+ (cd D &&
+ git alternate -a ../C/.git/objects
+ )
+'
+
+test_expect_success \
+ 'test git alternate display' '
+ testbase=$PWD
+ (cd B &&
+ git alternate >actual &&
+ {
+ echo "Object store $testbase/base/.git/objects"
+ echo " referenced via $testbase/B/.git"
+ } >expect &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success \
+ 'test git alternate recursive display' '
+ testbase=$PWD
+ (cd D &&
+
+ git alternate -r >actual &&
+
+ {
+ echo "Object store $testbase/C/.git/objects"
+ echo " referenced via $testbase/D/.git"
+ echo "Object store $testbase/base/.git/objects"
+ echo " referenced via $testbase/C/.git"
+ echo "Object store $testbase/B/.git/objects"
+ echo " referenced via $testbase/C/.git"
+ echo "Object store $testbase/base/.git/objects"
+ echo " referenced via $testbase/B/.git"
+ } >expect &&
+
+ test_cmp expect actual
+ )
+'
+
+#rm -rf A B C D base
\ No newline at end of file
--
1.7.0.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCHv2 2/2] tests for git alternate command
2010-03-25 6:07 ` [PATCHv2 2/2] tests for " Chris Packham
@ 2010-03-25 7:38 ` Johannes Sixt
2010-03-25 18:51 ` Chris Packham
0 siblings, 1 reply; 17+ messages in thread
From: Johannes Sixt @ 2010-03-25 7:38 UTC (permalink / raw)
To: Chris Packham; +Cc: git, jrnieder, gitster, bebarino
Am 3/25/2010 7:07, schrieb Chris Packham:
> Signed-off-by: Chris Packham <judge.packham@gmail.com>
Your patches violate whitespace rules. Use 'git show --check' to see the
questionable lines.
> ---
> I wasn't sure about the test numbering so I just grabbed the highest one. Still
> need to add tests for the deletion use case.
According to t/README, t1* would be a suitable category, perhaps t1430.
> t/t9800-git-alternate.sh | 95 ++++++++++++++++++++++++++++++++++++++++++++++
"git" in the name is redundant
> +test_expect_success \
> + 'Setup for rest of the test' '
The modern style for test headers is
test_expect_success 'Setup for rest of the test' '
test goes here
'
It may improve readability if you insert a blank line after the headline.
> + mkdir -p base &&
> + cd base &&
> + git init &&
> + echo test > a.txt &&
> + echo test > b.txt &&
> + echo test > c.txt &&
> + git add *.txt &&
> + git commit -a -m "Initial Commit" &&
> + cd .. &&
Do not use 'cd dir && ... && cd ..', use (cd dir && ...) like you did in
the rest of the tests.
> +test_expect_success \
> + 'Add alternate after clone' '
> + (cd B &&
> + git alternate -a ../base/.git/objects
> + )
We saw tests like this written with more whitespace like:
(
cd B &&
git alternate -a ../base/.git/objects
)
> +test_expect_success \
> + 'add same alternate fails adding existing abs path' '
> + (cd B &&
> + test_must_fail git alternate -a $PWD/base/.git/objects
This use of $PWD is OK, but for consistency it should be $(pwd) like
below. Moreover, it needs double-quotes.
> +test_expect_success \
> + 'test git alternate display' '
> + testbase=$PWD
You must write this as (d-quotes not needed here)
testbase=$(pwd) &&
for the benefit of Windows. The difference is that $PWD returns /c/path,
but $(pwd) returns c:/path.
When the alternate was set up using the command, git has only ever seen a
c:/path style path (regardless of whether you used $PWD or $(pwd), because
the path is converted to c:/path by the shell before it invokes git), and
therefore the alternates file contains this style.
But when the expected result is constructed, the /c/path style is *not*
converted to c:/path; and a mismatch would be detected.
> + (cd B &&
> + git alternate >actual &&
> + {
> + echo "Object store $testbase/base/.git/objects"
> + echo " referenced via $testbase/B/.git"
> + } >expect &&
> + test_cmp expect actual
> + )
> +'
> +
> +test_expect_success \
> + 'test git alternate recursive display' '
> + testbase=$PWD
Ditto.
> +#rm -rf A B C D base
> \ No newline at end of file
test_done
-- Hannes
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2 2/2] tests for git alternate command
2010-03-25 7:38 ` Johannes Sixt
@ 2010-03-25 18:51 ` Chris Packham
2010-03-26 0:48 ` Miklos Vajna
2010-03-26 6:44 ` Johannes Sixt
0 siblings, 2 replies; 17+ messages in thread
From: Chris Packham @ 2010-03-25 18:51 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, jrnieder, gitster, bebarino
On Thu, Mar 25, 2010 at 12:38 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 3/25/2010 7:07, schrieb Chris Packham:
>> Signed-off-by: Chris Packham <judge.packham@gmail.com>
>
> Your patches violate whitespace rules. Use 'git show --check' to see the
> questionable lines.
>
Will do. BTW theres no mention of --check in git show --help, but one
itch at a time.
>> ---
>> I wasn't sure about the test numbering so I just grabbed the highest one. Still
>> need to add tests for the deletion use case.
>
> According to t/README, t1* would be a suitable category, perhaps t1430.
>
>> t/t9800-git-alternate.sh | 95 ++++++++++++++++++++++++++++++++++++++++++++++
>
> "git" in the name is redundant
>
Is t1430-alternate-cmd.sh OK with everyone?
>> +test_expect_success \
>> + 'Setup for rest of the test' '
>
> The modern style for test headers is
>
[snip]
Will incorporate in next update
>> +#rm -rf A B C D base
>> \ No newline at end of file
>
> test_done
>
Ah, I knew it would be something simple like that. I guess I should
have started by reading t/README.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2 2/2] tests for git alternate command
2010-03-25 18:51 ` Chris Packham
@ 2010-03-26 0:48 ` Miklos Vajna
2010-03-26 6:44 ` Johannes Sixt
1 sibling, 0 replies; 17+ messages in thread
From: Miklos Vajna @ 2010-03-26 0:48 UTC (permalink / raw)
To: Chris Packham; +Cc: Johannes Sixt, git, jrnieder, gitster, bebarino
[-- Attachment #1: Type: text/plain, Size: 556 bytes --]
On Thu, Mar 25, 2010 at 11:51:15AM -0700, Chris Packham <judge.packham@gmail.com> wrote:
> On Thu, Mar 25, 2010 at 12:38 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> > Am 3/25/2010 7:07, schrieb Chris Packham:
> >> Signed-off-by: Chris Packham <judge.packham@gmail.com>
> >
> > Your patches violate whitespace rules. Use 'git show --check' to see the
> > questionable lines.
> >
>
> Will do. BTW theres no mention of --check in git show --help, but one
> itch at a time.
See http://thread.gmane.org/gmane.comp.version-control.git/80442
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2 2/2] tests for git alternate command
2010-03-25 18:51 ` Chris Packham
2010-03-26 0:48 ` Miklos Vajna
@ 2010-03-26 6:44 ` Johannes Sixt
1 sibling, 0 replies; 17+ messages in thread
From: Johannes Sixt @ 2010-03-26 6:44 UTC (permalink / raw)
To: Chris Packham; +Cc: git, jrnieder, gitster, bebarino
Am 3/25/2010 19:51, schrieb Chris Packham:
> Is t1430-alternate-cmd.sh OK with everyone?
It's fine.
-- Hannes
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2 1/2] Add git alternate command
2010-03-25 6:07 ` [PATCHv2 1/2] Add git alternate command Chris Packham
@ 2010-03-29 7:32 ` Junio C Hamano
2010-03-31 4:35 ` Chris Packham
0 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2010-03-29 7:32 UTC (permalink / raw)
To: Chris Packham; +Cc: git, jrnieder, gitster, bebarino
Chris Packham <judge.packham@gmail.com> writes:
> +abspath()
> +{
> + cd "$1"
> + pwd
> + cd - > /dev/null
> +}
I do not think "cd -" is all that portable. As you will be always using this in
this form:
somevariable=$(abspath "$it")
you are in a subshell and won't affect the caller anyway. Why not drop
the "go back to where we came from"?
Also a shell built-in "pwd" tends to be fooled by $PWD especially since
you are running "cd" without -P.
> +#
> +# 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()
This is more like "for_each_alternates"; "walk" can be mistaken as if you
are recursively looking at alternates defined in alternate repositories
of the repository you start from.
> +{
> + alternates=$GIT_DIR/objects/info/alternates
> + callback=$1
> + shift
> +
> + if test -f "$alternates"
> + then
> + while read line
> + do
> + $callback "$line" "$@"
> + done< "$alternates"
done <"$alternates"
How well does this handle relative alternate object stores? Shouldn't it
be more like this?
while read altdir
do
case "$altdir" in
/*) ;; # full path
*) altdir="$GIT_DIR/objects/$altdir" ;;
esac &&
$callback "$altdir" "$@"
done <"$alternates"
> +# 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"
> +
> + new_git_dir=${line%%/objects}
Do you need double-% here, not a single one?
> +# Add a new alternate
> +add_alternate()
> +{
> + if test ! -d "$dir"; then
> + die "fatal: $dir is not a directory"
> + fi
> +
> + abs_dir=$(abspath "$dir")
> + walk_alternates check_current_alternate_walk "$abs_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
s/its/(it's|it is)/;
But I don't think it is true that you have verified that it is not used.
You are running abspath on the input from the end user, but you are using
existing entries in the alternates file that may not be absolute. They
can be relative to $GIT_DIR/objects/.
> + say " use 'git repack -adl' to remove duplicate objects"
Good.
> +# Deletes the name alternate from the alternates file.
> +# If there are no more alternates the alternates file will be removed
> +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
Hmm, I am afraid that this will end up training users to always say -f
without even thinking. Shouldn't this code be doing whatever necessary
steps to make sure this repository has all the necessary objects without
the named alternates and then removing the file? An easiest might be to
temporarily remove the entry and run fsck, perhaps?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCHv2 1/2] Add git alternate command
2010-03-29 7:32 ` Junio C Hamano
@ 2010-03-31 4:35 ` Chris Packham
0 siblings, 0 replies; 17+ messages in thread
From: Chris Packham @ 2010-03-31 4:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jrnieder, bebarino
Sorry, just realized I missed this email before sending my last update.
On Mon, Mar 29, 2010 at 12:32 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Chris Packham <judge.packham@gmail.com> writes:
>
>> +abspath()
>> +{
>> + cd "$1"
>> + pwd
>> + cd - > /dev/null
>> +}
>
> I do not think "cd -" is all that portable. As you will be always using this in
> this form:
>
> somevariable=$(abspath "$it")
>
> you are in a subshell and won't affect the caller anyway. Why not drop
> the "go back to where we came from"?
Fair enough. Should be safe.
> Also a shell built-in "pwd" tends to be fooled by $PWD especially since
> you are running "cd" without -P.
Are you saying I should switch to "echo $PWD"?
>> +#
>> +# 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()
>
> This is more like "for_each_alternates"; "walk" can be mistaken as if you
> are recursively looking at alternates defined in alternate repositories
> of the repository you start from.
Easy change.
>> +{
>> + alternates=$GIT_DIR/objects/info/alternates
>> + callback=$1
>> + shift
>> +
>> + if test -f "$alternates"
>> + then
>> + while read line
>> + do
>> + $callback "$line" "$@"
>> + done< "$alternates"
>
> done <"$alternates"
>
> How well does this handle relative alternate object stores?
Not well at all.
> Shouldn't it be more like this?
>
> while read altdir
> do
> case "$altdir" in
> /*) ;; # full path
> *) altdir="$GIT_DIR/objects/$altdir" ;;
> esac &&
> $callback "$altdir" "$@"
> done <"$alternates"
>
Thanks, I was wondering how to handle relative paths.
>> +# 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"
>> +
>> + new_git_dir=${line%%/objects}
>
> Do you need double-% here, not a single one?
>
Not if I'm not using a regex. I'll change it.
>> +# Add a new alternate
>> +add_alternate()
>> +{
>> + if test ! -d "$dir"; then
>> + die "fatal: $dir is not a directory"
>> + fi
>> +
>> + abs_dir=$(abspath "$dir")
>> + walk_alternates check_current_alternate_walk "$abs_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
>
> s/its/(it's|it is)/;
>
> But I don't think it is true that you have verified that it is not used.
> You are running abspath on the input from the end user, but you are using
> existing entries in the alternates file that may not be absolute. They
> can be relative to $GIT_DIR/objects/.
>
I'll add some actual verification
>> + say " use 'git repack -adl' to remove duplicate objects"
>
> Good.
>
>> +# Deletes the name alternate from the alternates file.
>> +# If there are no more alternates the alternates file will be removed
>> +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
>
> Hmm, I am afraid that this will end up training users to always say -f
> without even thinking. Shouldn't this code be doing whatever necessary
> steps to make sure this repository has all the necessary objects without
> the named alternates and then removing the file? An easiest might be to
> temporarily remove the entry and run fsck, perhaps?
>
Yeah I had the same fear. I was wondering how to proceed with this part.
I take it your suggestion is to update the alternates file and run git
fsck to see if any objects are missing then, if there are missing
objects, switch to the original alternates file run git repack -a to
fetch the missing objects and finally switch to the new alternates
file. I'll code something up for us to discuss.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2010-03-31 4:35 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-03-22 17:26 Question about .git/objects/info/alternates Chris Packham
2010-03-23 2:42 ` Jonathan Nieder
2010-03-24 18:53 ` Chris Packham
2010-03-24 19:23 ` Jonathan Nieder
2010-03-24 19:58 ` Junio C Hamano
2010-03-24 20:35 ` Chris Packham
2010-03-25 6:07 ` Chris Packham
2010-03-25 6:07 ` [PATCHv2 1/2] Add git alternate command Chris Packham
2010-03-29 7:32 ` Junio C Hamano
2010-03-31 4:35 ` Chris Packham
2010-03-25 6:07 ` [PATCHv2 2/2] tests for " Chris Packham
2010-03-25 7:38 ` Johannes Sixt
2010-03-25 18:51 ` Chris Packham
2010-03-26 0:48 ` Miklos Vajna
2010-03-26 6:44 ` Johannes Sixt
2010-03-24 20:16 ` Question about .git/objects/info/alternates Stephen Boyd
2010-03-24 20:37 ` Chris Packham
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).