git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] object-file: use real paths when adding alternates

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Nov 17, 2022, 19:41 UTC
Message-ID
<221117.86h6yxgy7b.gmgdl@evledraar.gmail.com>
In-Reply-To
<Y3aBzbzub7flQyca@coredump.intra.peff.net>
On Thu, Nov 17 2022, Jeff King wrote:
Show 77 quoted lines
> On Thu, Nov 17, 2022 at 05:31:13PM +0000, Glen Choo via GitGitGadget wrote:
> [...]
>> @@ -596,7 +603,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,
>>  		return;
>>  	}
>>  
>> -	strbuf_add_absolute_path(&objdirbuf, r->objects->odb->path);
>> +	strbuf_realpath(&objdirbuf, r->objects->odb->path, 1);
>>  	if (strbuf_normalize_path(&objdirbuf) < 0)
>>  		die(_("unable to normalize object directory: %s"),
>>  		    objdirbuf.buf);
>
> Similarly here, I think we'd want to _replace_ the normalize with a
> realpath. There's no point in doing both. It's OK to die in this one
> because we assume the object directory can be normalized/realpath'd.
>
> So I'd have expected the code portion of your patch to be more like:
>
> diff --git a/object-file.c b/object-file.c
> index 957790098f..c6a195c6dd 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -508,6 +508,7 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
>  {
>  	struct object_directory *ent;
>  	struct strbuf pathbuf = STRBUF_INIT;
> +	struct strbuf tmp = STRBUF_INIT;
>  	khiter_t pos;
>  
>  	if (!is_absolute_path(entry->buf) && relative_base) {
> @@ -516,12 +517,18 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
>  	}
>  	strbuf_addbuf(&pathbuf, entry);
>  
> -	if (strbuf_normalize_path(&pathbuf) < 0 && relative_base) {
> -		error(_("unable to normalize alternate object path: %s"),
> -		      pathbuf.buf);
> -		strbuf_release(&pathbuf);
> -		return -1;
> +	if (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {
> +		if (relative_base) {
> +			error(_("unable to normalize alternate object path: %s"),
> +			      pathbuf.buf);
> +			strbuf_release(&pathbuf);
> +			return -1;
> +		}
> +		/* allow broken paths from env per 37a95862c625 */
> +		strbuf_addstr(&tmp, pathbuf.buf);
>  	}
> +	strbuf_swap(&pathbuf, &tmp);
> +	strbuf_release(&tmp);
>  
>  	/*
>  	 * The trailing slash after the directory name is given by
> @@ -596,10 +603,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,
>  		return;
>  	}
>  
> -	strbuf_add_absolute_path(&objdirbuf, r->objects->odb->path);
> -	if (strbuf_normalize_path(&objdirbuf) < 0)
> -		die(_("unable to normalize object directory: %s"),
> -		    objdirbuf.buf);
> +	strbuf_realpath(&objdirbuf, r->objects->odb->path, 1);
>  
>  	while (*alt) {
>  		alt = parse_alt_odb_entry(alt, sep, &entry);
>
> The "tmp" swapping in link_alt_odb_entry is kind of unfortunate. It
> would be nice if there were an in-place version of strbuf_realpath, even
> if it was using two buffers under the hood (which is how the normalize
> code does it). And then the patch really would be s/normalize/realpath/,
> which is easier to understand.
>
> Possibly this should also be using the "forgiving" version. We
> eventually error out on missing entries later on, so it's not a big deal
> to error here. But it would let us keep the error message the same. I
> don't know that it matters much in practice.

This probably isn't worth it, but I wondered if this wouldn't be easier if we pulled that memory management into the caller, it's not performance sensitive (or maybe, how many alternatives do people have :)), but an advantage of this is that we avoid the free()/malloc() if we only get partway through, i.e. return early and keep looping.

In terms of general code smell & how we manage the "return" here, as adding "RESULT_MUST_BE_USED" to this shows we never use the "0" or "-1" (or any other...) return value.

That's been the case since this was added in c2f493a4ae1 (Transitively read alternatives, 2006-05-07), so we can probably just make this a "void" and ditch the returns if we're finding ourselves juggling these return values...

 object-file.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/object-file.c b/object-file.c
index c6a195c6dd2..1a94d98e0c7 100644
--- a/object-file.c
+++ b/object-file.c
@@ -504,47 +504,43 @@ static void read_info_alternates(struct repository *r,
 				 const char *relative_base,
 				 int depth);
 static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry,
-	const char *relative_base, int depth, const char *normalized_objdir)
+			      const char *relative_base, int depth,
+			      const char *normalized_objdir,
+			      struct strbuf *pathbuf)
 {
 	struct object_directory *ent;
-	struct strbuf pathbuf = STRBUF_INIT;
 	struct strbuf tmp = STRBUF_INIT;
 	khiter_t pos;
 
 	if (!is_absolute_path(entry->buf) && relative_base) {
-		strbuf_realpath(&pathbuf, relative_base, 1);
-		strbuf_addch(&pathbuf, '/');
+		strbuf_realpath(pathbuf, relative_base, 1);
+		strbuf_addch(pathbuf, '/');
 	}
-	strbuf_addbuf(&pathbuf, entry);
+	strbuf_addbuf(pathbuf, entry);
 
-	if (!strbuf_realpath(&tmp, pathbuf.buf, 0)) {
-		if (relative_base) {
-			error(_("unable to normalize alternate object path: %s"),
-			      pathbuf.buf);
-			strbuf_release(&pathbuf);
-			return -1;
-		}
+	if (!strbuf_realpath(&tmp, pathbuf->buf, 0)) {
+		if (relative_base)
+			return error(_("unable to normalize alternate object path: %s"),
+				     pathbuf->buf);
 		/* allow broken paths from env per 37a95862c625 */
-		strbuf_addstr(&tmp, pathbuf.buf);
+		strbuf_addstr(&tmp, pathbuf->buf);
 	}
-	strbuf_swap(&pathbuf, &tmp);
+	strbuf_swap(pathbuf, &tmp);
 	strbuf_release(&tmp);
 
 	/*
 	 * The trailing slash after the directory name is given by
 	 * this function at the end. Remove duplicates.
 	 */
-	while (pathbuf.len && pathbuf.buf[pathbuf.len - 1] == '/')
-		strbuf_setlen(&pathbuf, pathbuf.len - 1);
+	while (pathbuf->len && pathbuf->buf[pathbuf->len - 1] == '/')
+		strbuf_setlen(pathbuf, pathbuf->len - 1);
 
-	if (!alt_odb_usable(r->objects, &pathbuf, normalized_objdir, &pos)) {
-		strbuf_release(&pathbuf);
+	if (!alt_odb_usable(r->objects, pathbuf, normalized_objdir, &pos))
 		return -1;
-	}
 
 	CALLOC_ARRAY(ent, 1);
-	/* pathbuf.buf is already in r->objects->odb_by_path */
-	ent->path = strbuf_detach(&pathbuf, NULL);
+	/* pathbuf->buf is already in r->objects->odb_by_path */
+	ent->path = strbuf_detach(pathbuf, NULL);
 
 	/* add the alternate entry */
 	*r->objects->odb_tail = ent;
@@ -593,6 +589,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,
 {
 	struct strbuf objdirbuf = STRBUF_INIT;
 	struct strbuf entry = STRBUF_INIT;
+	struct strbuf pathbuf = STRBUF_INIT;
 
 	if (!alt || !*alt)
 		return;
@@ -610,8 +607,11 @@ static void link_alt_odb_entries(struct repository *r, const char *alt,
 		if (!entry.len)
 			continue;
 		link_alt_odb_entry(r, &entry,
-				   relative_base, depth, objdirbuf.buf);
+				   relative_base, depth, objdirbuf.buf,
+				   &pathbuf);
+		strbuf_reset(&pathbuf);
 	}
+	strbuf_release(&pathbuf);
 	strbuf_release(&entry);
 	strbuf_release(&objdirbuf);
 }
Previous: Jeff KingNext: Jeff King
Message 3 of 17 in “object-file: use real paths when adding alternates”
  1. object-file: use real paths when adding alternatesGlen Choo via GitGitGadget, Nov 17, 2022
  2. Jeff KingNov 17, 2022
  3. Ævar Arnfjörð BjarmasonNov 17, 2022
  4. Jeff KingNov 17, 2022
  5. Taylor BlauNov 17, 2022
  6. Glen ChooNov 18, 2022
  7. Taylor BlauNov 17, 2022
  8. object-file: use real paths when adding alternatesGlen Choo via GitGitGadget, Nov 21, 2022
  9. Ævar Arnfjörð BjarmasonNov 22, 2022
  10. Jeff KingNov 22, 2022
  11. Glen ChooNov 24, 2022
  12. Jeff KingNov 24, 2022
  13. Glen ChooNov 24, 2022
  14. Jeff KingNov 22, 2022
  15. object-file: use real paths when adding alternatesGlen Choo via GitGitGadget, Nov 24, 2022
  16. Jeff KingNov 24, 2022
  17. Junio C HamanoNov 25, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.