Re: [PATCHv2 1/2] Add git alternate command

[Chris Packham <judge.packham@gmail.com>]

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.