Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors

> On Mon, Nov 07, 2016 at 03:42:35PM -0800, Bryan Turner wrote:
>
>> > @@ -335,7 +340,9 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,
>> >         }
>> >
>> >         strbuf_add_absolute_path(&objdirbuf, get_object_directory());
>> > -       normalize_path_copy(objdirbuf.buf, objdirbuf.buf);
>> > +       if (strbuf_normalize_path(&objdirbuf) < 0)
>> > +               die("unable to normalize object directory: %s",
>> > +                   objdirbuf.buf);
>>
>> This appears to break the ability to use a relative alternate via an
>> environment variable, since normalize_path_copy_len is explicitly
>> documented "Returns failure (non-zero) if a ".." component appears as
>> first path"
>
> That shouldn't happen, though, because the path we are normalizing has
> been converted to an absolute path via strbuf_add_absolute_path. IOW, if
> your relative path is "../../../foo", we should be feeding something
> like "/path/to/repo/.git/objects/../../../foo" and normalizing that to
> "/path/to/foo".
>
> But in your example, you see:
>
>   error: unable to normalize alternate object path: ../0/objects
>
> which cannot come from the code above, which calls die(). It should be
> coming from the call in link_alt_odb_entry().

> No, I had no intention of disallowing relative alternates (and as you
> noticed, a commit from the same series actually expands the use of
> relative alternates). My use has been entirely within info/alternates
> files, though, not via the environment.
>
> As I said, I'm not sure this was ever meant to work, but as far as I can
> tell it mostly _has_ worked, modulo some quirks. So I think we should
> consider it a regression for it to stop working in v2.11.
>
> The obvious solution is one of:
>
>   1. Stop calling normalize() at all when we do not have a relative base
>      and the path is not absolute. This restores the original quirky
>      behavior (plus makes the "foo/../../bar" case work).
>
>      If we want to do the minimum before releasing v2.11, it would be
>      that. I'm not sure it leaves things in a very sane state, but at
>      least v2.11 does no harm, and anybody who cares can build saner
>      semantics for v2.12.
>
>   2. Fix it for real. Pass a real relative_base when linking from the
>      environment. The question is: what is the correct relative base? I
>      suppose "getcwd() at the time we prepare the alt odb" is
>      reasonable, and would behave similarly to older versions ($GIT_DIR
>      for bare repos, top of the working tree otherwise).
>
>      If we were designing from scratch, I think saner semantics would
>      probably be always relative from $GIT_DIR, or even always relative
>      from the object directory (i.e., behave as if the paths were given
>      in objects/info/alternates). But that breaks compatibility with
>      older versions. If we are treating this as a regression, it is not
>      very friendly to say "you are still broken, but you might just need
>      to add an extra '..' to your path".
>
> So I dunno. I guess that inclines me towards (1), as it lets us punt on
> the harder question.