On Mon, Jul 14, 2014 at 4:02 PM, Ephrim Khong <dr.kh...@gmail.com> wrote:
> When adding alternate object directories, we try not to add the
> directory of the current repository to avoid cycles.  Unfortunately,
> that test was broken, since it compared an absolute with a relative
> path.

Not blaming anyone. I'm simply interested in code archeology. The
first introduction of this strcmp is from 1494e03 (sha1_file.c: make
sure packs in an alternate odb is named properly. - 2005-12-04). The
intention is good as described in the commit message. But I think it's
broken even back then because "objdir" (or get_object_directory())
will always be relative (unless someone sets it explicitly) and
ent->base back then is already absolute (but not normalized).

> Signed-off-by: Ephrim Khong <dr.kh...@gmail.com>
> ---
> My first patch, so be harsh.  I'm not sure about the filename of the test,
> the behavior is tested with repack, but it affects gc and others as well.
>  sha1_file.c                        |  2 +-
>  t/t7702-repack-cyclic-alternate.sh | 24 ++++++++++++++++++++++++
>  2 files changed, 25 insertions(+), 1 deletion(-)
>  create mode 100755 t/t7702-repack-cyclic-alternate.sh
> diff --git a/sha1_file.c b/sha1_file.c
> index 34d527f..7e98e9e 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -320,7 +320,7 @@ static int link_alt_odb_entry(const char *entry, const
> char *relative_base, int
>                         return -1;
>                 }
>         }
> -       if (!strcmp(ent->base, objdir)) {
> +       if (!strcmp(ent->base, absolute_path(objdir))) {

I think you want to normalize objdir in addition to making it
absolute, in case someone sets GIT_OBJECT_DIR to foo///bar. ent->base
is normalized already. Oh and maybe use strcmp_icase to be nice on
case-insensitive filesystems..

Kinda nit picking, but perhaps this path
absolute-ization/normalization should be done by the caller
link_alt_odb_entries so you don't have to redo it for every entry in
alternates file. I know there's a loop of memcmp() right above this
that may be more expensive than this micro-optimization when we have
lots of alternate entries.
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to