Re: [PATCH] sha1_file: do not add own object directory as alternate
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Jul 14, 2014, 10:47 UTC
- Message-ID
- <CACsJy8B-QFu4SU6HHZukAmmjLrgb+L8KsHPn3GgQhcX6-LGXLg@mail.gmail.com>
- In-Reply-To
- <53C39CB0.6040909@gmail.com>
On Mon, Jul 14, 2014 at 4:02 PM, Ephrim Khong <dr.khong@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).
Show 22 quoted lines
>
> Signed-off-by: Ephrim Khong <dr.khong@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.
-- Duy