threads / patch / 28267

patchsha1_file: Remove relative entries limitation

Subject: [PATCH] sha1_file: Remove relative entries limitation

## tl;dr

3 messages between Aug 31, 2011 and Sep 2, 2011. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Hui Wang· Aug 31, 2011, 06:41 UTC · lore

link_alt_odb_entries() will be called recursively if alternates has valid object store paths, and to avoid nesting too deep, the recursive depth is limited to 5, this limitation is reasonable and safe for dead-loop reference situation.

There is another limitation in this function to only permit the 1st level alternates has relative paths, but there is no foreseeable greater risk using relative paths in 2nd/3rd... level alternates than using absolute paths, in addition to we already have max depth 5 limitation, we can safely remove this limitation.

Moreover removing this limitation will make below two usage workable.
usage1: base-repos has relative path in the alternates
        %>git clone --reference base-repos src dest
usage2: src2 has relative path to point src1, src1 has relative path
    	to point src
        %>git clone src2 dest
Signed-off-by: Hui Wang <jason77.wang@gmail.com>
---
 sha1_file.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)
Show changes to sha1_file.c +4 −9
diff --git a/sha1_file.c b/sha1_file.c
index f7c3408..4130ca0 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -332,15 +332,10 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,
 		}
 		while (cp < ep && *cp != sep)
 			cp++;
-		if (last != cp) {
-			if (!is_absolute_path(last) && depth) {
-				error("%s: ignoring relative alternate object store %s",
-						relative_base, last);
-			} else {
-				link_alt_odb_entry(last, cp - last,
-						relative_base, depth);
-			}
-		}
+		if (last != cp)
+			link_alt_odb_entry(last, cp - last,
+					relative_base, depth);
+
 		while (cp < ep && *cp == sep)
 			cp++;
 		last = cp;
-- 
1.7.6
Hui Wang· Sep 2, 2011, 09:49 UTC · re: Hui Wang · lore

Re: [PATCH] sha1_file: Remove relative entries limitation

Hi Junio,
Does this patch make sense?

Regards, Hui Wang.

Hui Wang wrote:
Show 49 quoted lines
> link_alt_odb_entries() will be called recursively if alternates has
> valid object store paths, and to avoid nesting too deep, the
> recursive depth is limited to 5, this limitation is reasonable and
> safe for dead-loop reference situation.
>
> There is another limitation in this function to only permit the 1st
> level alternates has relative paths, but there is no foreseeable
> greater risk using relative paths in 2nd/3rd... level alternates than
> using absolute paths, in addition to we already have max depth 5
> limitation, we can safely remove this limitation.
>
> Moreover removing this limitation will make below two usage workable.
>
> usage1: base-repos has relative path in the alternates
>         %>git clone --reference base-repos src dest
> usage2: src2 has relative path to point src1, src1 has relative path
>     	to point src
>         %>git clone src2 dest
>
> Signed-off-by: Hui Wang <jason77.wang@gmail.com>
> ---
>  sha1_file.c |   13 ++++---------
>  1 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index f7c3408..4130ca0 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -332,15 +332,10 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,
>  		}
>  		while (cp < ep && *cp != sep)
>  			cp++;
> -		if (last != cp) {
> -			if (!is_absolute_path(last) && depth) {
> -				error("%s: ignoring relative alternate object store %s",
> -						relative_base, last);
> -			} else {
> -				link_alt_odb_entry(last, cp - last,
> -						relative_base, depth);
> -			}
> -		}
> +		if (last != cp)
> +			link_alt_odb_entry(last, cp - last,
> +					relative_base, depth);
> +
>  		while (cp < ep && *cp == sep)
>  			cp++;
>  		last = cp;
>   
Junio C Hamano· Sep 2, 2011, 16:22 UTC · re: Hui Wang · lore

Re: [PATCH] sha1_file: Remove relative entries limitation

Hui Wang <jason77.wang@gmail.com> writes:
> Does this patch make sense?

I do not see anything wrong with it per-se, but I'd like to see somebody made an effort to dig why the restriction is there in the first place. The check is very much deliberately placed there and there may be a good reason why we do not resolve relatives of indirect alternates that you are missing when you wrote the patch (and I am missing when I wrote this response).

← back to recent threads