threads / patch / 55121

patch, 2 partswrite_entry(): fix misuses of `path` in error messages

Subject: [PATCH 1/2] write_entry(): fix misuses of `path` in error messages

## tl;dr

15 messages between Feb 8, 2021 and Feb 16, 2021. Diffs are folded; open one to read it.

replies: 14people: 2as markdown or json

Matheus Tavares· Feb 8, 2021, 19:36 UTC · lore

[PATCH 0/2] checkout-index: some cleanups to --temp and --prefix outputs

These are some minor cleanups to the checkout-index output when using --temp or --prefix. The first patch fixes a few wrong uses of `path` in write_entry() when it should be `ce->name`, and the second patch removes malformed entries from the --temp output list.

Matheus Tavares (2):
  write_entry(): fix misuses of `path` in error messages
  checkout-index: omit entries with no tempname from --temp output

 builtin/checkout-index.c | 39 ++++++++++++++++++++++++++-------------
 entry.c                  |  8 ++++----
 2 files changed, 30 insertions(+), 17 deletions(-)

-- 
2.29.2

Matheus Tavares· Feb 8, 2021, 19:36 UTC · re: Matheus Tavares · lore

The variables `path` and `ce->name`, at write_entry(), usually have the same contents, but that's not the case when using a checkout prefix or writing to a tempfile. (In fact, `path` will be either empty or dirty when writing to a tempfile.) Therefore, these variables cannot be used interchangeably. In this sense, fix wrong uses of `path` in error messages where it should really be `ce->name`. (There doesn't seem to be any misuse in the other way around.)

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 entry.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Show changes to entry.c +4 −4

diff --git a/entry.c b/entry.c
index a0532f1f00..7b9f43716f 100644
--- a/entry.c
+++ b/entry.c
@@ -282,7 +282,7 @@ static int write_entry(struct cache_entry *ce,
 		new_blob = read_blob_entry(ce, &size);
 		if (!new_blob)
 			return error("unable to read sha1 file of %s (%s)",
-				     path, oid_to_hex(&ce->oid));
+				     ce->name, oid_to_hex(&ce->oid));
 
 		/*
 		 * We can't make a real symlink; write out a regular file entry
@@ -309,7 +309,7 @@ static int write_entry(struct cache_entry *ce,
 			new_blob = read_blob_entry(ce, &size);
 			if (!new_blob)
 				return error("unable to read sha1 file of %s (%s)",
-					     path, oid_to_hex(&ce->oid));
+					     ce->name, oid_to_hex(&ce->oid));
 		}
 
 		/*
@@ -354,7 +354,7 @@ static int write_entry(struct cache_entry *ce,
 
 	case S_IFGITLINK:
 		if (to_tempfile)
-			return error("cannot create temporary submodule %s", path);
+			return error("cannot create temporary submodule %s", ce->name);
 		if (mkdir(path, 0777) < 0)
 			return error("cannot create submodule directory %s", path);
 		sub = submodule_from_ce(ce);
@@ -365,7 +365,7 @@ static int write_entry(struct cache_entry *ce,
 		break;
 
 	default:
-		return error("unknown file mode for %s in index", path);
+		return error("unknown file mode for %s in index", ce->name);
 	}
 
 finish:

-- 
2.29.2

Junio C Hamano· Feb 9, 2021, 21:27 UTC · re: Matheus Tavares · lore

Re: [PATCH 1/2] write_entry(): fix misuses of `path` in error messages

Matheus Tavares <matheus.bernardino@usp.br> writes:

Show 7 quoted lines

> The variables `path` and `ce->name`, at write_entry(), usually have the
> same contents, but that's not the case when using a checkout prefix or
> writing to a tempfile. (In fact, `path` will be either empty or dirty
> when writing to a tempfile.) Therefore, these variables cannot be used
> interchangeably. In this sense, fix wrong uses of `path` in error
> messages where it should really be `ce->name`. (There doesn't seem to be
> any misuse in the other way around.)

Sounds reasonable.  Don't we want to protect this fix with tests?

Show 45 quoted lines

> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---
>  entry.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/entry.c b/entry.c
> index a0532f1f00..7b9f43716f 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -282,7 +282,7 @@ static int write_entry(struct cache_entry *ce,
>  		new_blob = read_blob_entry(ce, &size);
>  		if (!new_blob)
>  			return error("unable to read sha1 file of %s (%s)",
> -				     path, oid_to_hex(&ce->oid));
> +				     ce->name, oid_to_hex(&ce->oid));
>  
>  		/*
>  		 * We can't make a real symlink; write out a regular file entry
> @@ -309,7 +309,7 @@ static int write_entry(struct cache_entry *ce,
>  			new_blob = read_blob_entry(ce, &size);
>  			if (!new_blob)
>  				return error("unable to read sha1 file of %s (%s)",
> -					     path, oid_to_hex(&ce->oid));
> +					     ce->name, oid_to_hex(&ce->oid));
>  		}
>  
>  		/*
> @@ -354,7 +354,7 @@ static int write_entry(struct cache_entry *ce,
>  
>  	case S_IFGITLINK:
>  		if (to_tempfile)
> -			return error("cannot create temporary submodule %s", path);
> +			return error("cannot create temporary submodule %s", ce->name);
>  		if (mkdir(path, 0777) < 0)
>  			return error("cannot create submodule directory %s", path);
>  		sub = submodule_from_ce(ce);
> @@ -365,7 +365,7 @@ static int write_entry(struct cache_entry *ce,
>  		break;
>  
>  	default:
> -		return error("unknown file mode for %s in index", path);
> +		return error("unknown file mode for %s in index", ce->name);
>  	}
>  
>  finish:

Matheus Tavares Bernardino· Feb 9, 2021, 23:59 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] write_entry(): fix misuses of `path` in error messages

On Tue, Feb 9, 2021 at 6:27 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 12 quoted lines

>
> Matheus Tavares <matheus.bernardino@usp.br> writes:
>
> > The variables `path` and `ce->name`, at write_entry(), usually have the
> > same contents, but that's not the case when using a checkout prefix or
> > writing to a tempfile. (In fact, `path` will be either empty or dirty
> > when writing to a tempfile.) Therefore, these variables cannot be used
> > interchangeably. In this sense, fix wrong uses of `path` in error
> > messages where it should really be `ce->name`. (There doesn't seem to be
> > any misuse in the other way around.)
>
> Sounds reasonable.  Don't we want to protect this fix with tests?

Yeah, good idea. I will add a couple tests to check the error message on missing blobs and when trying to write a submodule to a tempfile. This should cover 3 out of the 4 error() calls changed in this patch. (The last one would be when ce->mode is unknown. I'm not sure if/how I can trigger that case.)

Matheus Tavares· Feb 8, 2021, 19:36 UTC · re: Matheus Tavares · lore

[PATCH 2/2] checkout-index: omit entries with no tempname from --temp output

With --temp (or --stage=all, which implies --temp), checkout-index writes a list to stdout associating temporary file names to the entries' names. But if it fails to write an entry, and the failure happens before even assigning a temporary filename to that entry, we get an odd output line. This can be seen when trying to check out a symlink whose blob is missing:

$ missing_blob=$(git hash-object --stdin </dev/null)
$ git update-index --add --cacheinfo 120000,$missing_blob,foo
$ git checkout-index --temp foo
error: unable to read sha1 file of foo (e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)
        foo

The 'TAB foo' line is not much useful and it might break scripts that expect the 'tempname TAB foo' output. So let's omit such entries from the stdout list (but leaving the error message on stderr).

We could also consider omitting _all_ failed entries from the output list, but that's probably not a good idea as the associated tempfiles may have been created even when checkout failed, so scripts may want to use the output list for cleanup.

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 builtin/checkout-index.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

Show changes to builtin/checkout-index.c +26 −13

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 4bbfc92dce..a9f0f0a225 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -23,25 +23,38 @@ static struct checkout state = CHECKOUT_INIT;
 static void write_tempfile_record(const char *name, const char *prefix)
 {
 	int i;
+	int have_tempname = 0;
 
 	if (CHECKOUT_ALL == checkout_stage) {
-		for (i = 1; i < 4; i++) {
-			if (i > 1)
-				putchar(' ');
-			if (topath[i][0])
-				fputs(topath[i], stdout);
-			else
-				putchar('.');
+		for (i = 1; i < 4; i++)
+			if (topath[i][0]) {
+				have_tempname = 1;
+				break;
+			}
+
+		if (have_tempname) {
+			for (i = 1; i < 4; i++) {
+				if (i > 1)
+					putchar(' ');
+				if (topath[i][0])
+					fputs(topath[i], stdout);
+				else
+					putchar('.');
+			}
 		}
-	} else
+	} else if (topath[checkout_stage][0]) {
+		have_tempname = 1;
 		fputs(topath[checkout_stage], stdout);
+	}
 
-	putchar('\t');
-	write_name_quoted_relative(name, prefix, stdout,
-				   nul_term_line ? '\0' : '\n');
+	if (have_tempname) {
+		putchar('\t');
+		write_name_quoted_relative(name, prefix, stdout,
+					   nul_term_line ? '\0' : '\n');
 
-	for (i = 0; i < 4; i++) {
-		topath[i][0] = 0;
+		for (i = 0; i < 4; i++) {
+			topath[i][0] = 0;
+		}
 	}
 }

-- 
2.29.2

Junio C Hamano· Feb 9, 2021, 21:35 UTC · re: Matheus Tavares · lore

Re: [PATCH 2/2] checkout-index: omit entries with no tempname from --temp output

Matheus Tavares <matheus.bernardino@usp.br> writes:

Show 16 quoted lines

> With --temp (or --stage=all, which implies --temp), checkout-index
> writes a list to stdout associating temporary file names to the entries'
> names. But if it fails to write an entry, and the failure happens before
> even assigning a temporary filename to that entry, we get an odd output
> line. This can be seen when trying to check out a symlink whose blob is
> missing:
>
> $ missing_blob=$(git hash-object --stdin </dev/null)
> $ git update-index --add --cacheinfo 120000,$missing_blob,foo
> $ git checkout-index --temp foo
> error: unable to read sha1 file of foo (e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)
>         foo
>
> The 'TAB foo' line is not much useful and it might break scripts that
> expect the 'tempname TAB foo' output. So let's omit such entries from
> the stdout list (but leaving the error message on stderr).

Makes quite a lot of sense.

Show 44 quoted lines

>  	if (CHECKOUT_ALL == checkout_stage) {
> -		for (i = 1; i < 4; i++) {
> -			if (i > 1)
> -				putchar(' ');
> -			if (topath[i][0])
> -				fputs(topath[i], stdout);
> -			else
> -				putchar('.');
> +		for (i = 1; i < 4; i++)
> +			if (topath[i][0]) {
> +				have_tempname = 1;
> +				break;
> +			}
> +
> +		if (have_tempname) {
> +			for (i = 1; i < 4; i++) {
> +				if (i > 1)
> +					putchar(' ');
> +				if (topath[i][0])
> +					fputs(topath[i], stdout);
> +				else
> +					putchar('.');
> +			}
>  		}
> -	} else
> +	} else if (topath[checkout_stage][0]) {
> +		have_tempname = 1;
>  		fputs(topath[checkout_stage], stdout);
> +	}
>  
> -	putchar('\t');
> -	write_name_quoted_relative(name, prefix, stdout,
> -				   nul_term_line ? '\0' : '\n');
> +	if (have_tempname) {
> +		putchar('\t');
> +		write_name_quoted_relative(name, prefix, stdout,
> +					   nul_term_line ? '\0' : '\n');
>  
> -	for (i = 0; i < 4; i++) {
> -		topath[i][0] = 0;
> +		for (i = 0; i < 4; i++) {
> +			topath[i][0] = 0;
> +		}
>  	}

Hmph, is topath[][] array used after this function gets called and in what way? Whether have_tempname is true or not, wouldn't we want to clear it?

Thanks.

Matheus Tavares Bernardino· Feb 9, 2021, 21:57 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] checkout-index: omit entries with no tempname from --temp output

On Tue, Feb 9, 2021 at 6:35 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 51 quoted lines

>
> Matheus Tavares <matheus.bernardino@usp.br> writes:
>
> >       if (CHECKOUT_ALL == checkout_stage) {
> > -             for (i = 1; i < 4; i++) {
> > -                     if (i > 1)
> > -                             putchar(' ');
> > -                     if (topath[i][0])
> > -                             fputs(topath[i], stdout);
> > -                     else
> > -                             putchar('.');
> > +             for (i = 1; i < 4; i++)
> > +                     if (topath[i][0]) {
> > +                             have_tempname = 1;
> > +                             break;
> > +                     }
> > +
> > +             if (have_tempname) {
> > +                     for (i = 1; i < 4; i++) {
> > +                             if (i > 1)
> > +                                     putchar(' ');
> > +                             if (topath[i][0])
> > +                                     fputs(topath[i], stdout);
> > +                             else
> > +                                     putchar('.');
> > +                     }
> >               }
> > -     } else
> > +     } else if (topath[checkout_stage][0]) {
> > +             have_tempname = 1;
> >               fputs(topath[checkout_stage], stdout);
> > +     }
> >
> > -     putchar('\t');
> > -     write_name_quoted_relative(name, prefix, stdout,
> > -                                nul_term_line ? '\0' : '\n');
> > +     if (have_tempname) {
> > +             putchar('\t');
> > +             write_name_quoted_relative(name, prefix, stdout,
> > +                                        nul_term_line ? '\0' : '\n');
> >
> > -     for (i = 0; i < 4; i++) {
> > -             topath[i][0] = 0;
> > +             for (i = 0; i < 4; i++) {
> > +                     topath[i][0] = 0;
> > +             }
> >       }
>
> Hmph, is topath[][] array used after this function gets called and
> in what way?  Whether have_tempname is true or not, wouldn't we want
> to clear it?

Yeah, topath[][] can be reused in the next checkout_entry() call. But if have_tempname is false, the positions that are going to be used again (either checkout_stage or 1, 2, and 3, if checkout_stage == CHECKOUT_ALL) will be already empty. So I think we only need to clear topath[][] when have_tempname is false.

Junio C Hamano· Feb 10, 2021, 01:07 UTC · re: Matheus Tavares Bernardino · lore

Re: [PATCH 2/2] checkout-index: omit entries with no tempname from --temp output

Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:

Show 9 quoted lines

>> Hmph, is topath[][] array used after this function gets called and
>> in what way?  Whether have_tempname is true or not, wouldn't we want
>> to clear it?
>
> Yeah, topath[][] can be reused in the next checkout_entry() call. But
> if have_tempname is false, the positions that are going to be used
> again (either checkout_stage or 1, 2, and 3, if checkout_stage ==
> CHECKOUT_ALL) will be already empty. So I think we only need to clear
> topath[][] when have_tempname is false.

If so, clearing them unconditionally like the original code before the introduction of have_tempname variable would be easier on readers, as they won't be forced to reason about when to and when not to clear these strings---figure out if the reason why we do not always clear is because (1) we have info that we do not want to lose if (!have_tempname), or (2) we know there is nothing to be cleared if (!have_tempname).

Thanks.

Matheus Tavares· Feb 15, 2021, 18:24 UTC · re: Matheus Tavares · lore

[PATCH v2 0/2] checkout-index: some cleanups to --temp and --prefix outputs

Changes since v1:
- Added regression tests in first patch.
- Left the `topath[]` cleanup outside the if block on patch 2, to make
  it easier to read.

Matheus Tavares (2):
  write_entry(): fix misuses of `path` in error messages
  checkout-index: omit entries with no tempname from --temp output

 builtin/checkout-index.c        | 35 ++++++++++++++++++++++-----------
 entry.c                         |  8 ++++----
 t/t2006-checkout-index-basic.sh | 23 ++++++++++++++++++++++
 3 files changed, 51 insertions(+), 15 deletions(-)

Range-diff against v1:
1:  d52bcad326 ! 1:  bdda5f99d0 write_entry(): fix misuses of `path` in error messages
    @@ Commit message
         writing to a tempfile. (In fact, `path` will be either empty or dirty
         when writing to a tempfile.) Therefore, these variables cannot be used
         interchangeably. In this sense, fix wrong uses of `path` in error
    -    messages where it should really be `ce->name`. (There doesn't seem to be
    -    any misuse in the other way around.)
    +    messages where it should really be `ce->name`, and add some regression
    +    tests. (Note: there doesn't seem to be any misuse in the other way
    +    around.)
     
         Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
     
    @@ entry.c: static int write_entry(struct cache_entry *ce,
      	}
      
      finish:
    +
    + ## t/t2006-checkout-index-basic.sh ##
    +@@ t/t2006-checkout-index-basic.sh: test_expect_success 'checkout-index reports errors (stdin)' '
    + 	test_i18ngrep not.in.the.cache stderr
    + '
    + 
    ++test_expect_success 'checkout-index --temp correctly reports error on missing blobs' '
    ++	test_when_finished git reset --hard &&
    ++	missing_blob=$(git hash-object --stdin </dev/null) &&
    ++	cat >objs <<-EOF &&
    ++	100644 $missing_blob	file
    ++	120000 $missing_blob	symlink
    ++	EOF
    ++	git update-index --index-info <objs &&
    ++
    ++	test_must_fail git checkout-index --temp symlink file 2>stderr &&
    ++	test_i18ngrep "unable to read sha1 file of file ($missing_blob)" stderr &&
    ++	test_i18ngrep "unable to read sha1 file of symlink ($missing_blob)" stderr
    ++'
    ++
    ++test_expect_success 'checkout-index --temp correctly reports error for submodules' '
    ++	git init sub &&
    ++	test_commit -C sub file &&
    ++	git submodule add ./sub &&
    ++	git commit -m sub &&
    ++	test_must_fail git checkout-index --temp sub 2>stderr &&
    ++	test_i18ngrep "cannot create temporary submodule sub" stderr
    ++'
    ++
    + test_done
2:  1275701345 ! 2:  6ece1947c1 checkout-index: omit entries with no tempname from --temp output
    @@ builtin/checkout-index.c: static struct checkout state = CHECKOUT_INIT;
     +		putchar('\t');
     +		write_name_quoted_relative(name, prefix, stdout,
     +					   nul_term_line ? '\0' : '\n');
    ++	}
      
    --	for (i = 0; i < 4; i++) {
    --		topath[i][0] = 0;
    -+		for (i = 0; i < 4; i++) {
    -+			topath[i][0] = 0;
    -+		}
    - 	}
    - }
    - 
    + 	for (i = 0; i < 4; i++) {
    + 		topath[i][0] = 0;

-- 
2.29.2

Matheus Tavares· Feb 15, 2021, 18:24 UTC · re: Matheus Tavares · lore

[PATCH v2 1/2] write_entry(): fix misuses of `path` in error messages

The variables `path` and `ce->name`, at write_entry(), usually have the same contents, but that's not the case when using a checkout prefix or writing to a tempfile. (In fact, `path` will be either empty or dirty when writing to a tempfile.) Therefore, these variables cannot be used interchangeably. In this sense, fix wrong uses of `path` in error messages where it should really be `ce->name`, and add some regression tests. (Note: there doesn't seem to be any misuse in the other way around.)

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 entry.c                         |  8 ++++----
 t/t2006-checkout-index-basic.sh | 23 +++++++++++++++++++++++
 2 files changed, 27 insertions(+), 4 deletions(-)

Show changes to 2 files +27 −4

entry.c, t/t2006-checkout-index-basic.sh

diff --git a/entry.c b/entry.c
index a0532f1f00..7b9f43716f 100644
--- a/entry.c
+++ b/entry.c
@@ -282,7 +282,7 @@ static int write_entry(struct cache_entry *ce,
 		new_blob = read_blob_entry(ce, &size);
 		if (!new_blob)
 			return error("unable to read sha1 file of %s (%s)",
-				     path, oid_to_hex(&ce->oid));
+				     ce->name, oid_to_hex(&ce->oid));
 
 		/*
 		 * We can't make a real symlink; write out a regular file entry
@@ -309,7 +309,7 @@ static int write_entry(struct cache_entry *ce,
 			new_blob = read_blob_entry(ce, &size);
 			if (!new_blob)
 				return error("unable to read sha1 file of %s (%s)",
-					     path, oid_to_hex(&ce->oid));
+					     ce->name, oid_to_hex(&ce->oid));
 		}
 
 		/*
@@ -354,7 +354,7 @@ static int write_entry(struct cache_entry *ce,
 
 	case S_IFGITLINK:
 		if (to_tempfile)
-			return error("cannot create temporary submodule %s", path);
+			return error("cannot create temporary submodule %s", ce->name);
 		if (mkdir(path, 0777) < 0)
 			return error("cannot create submodule directory %s", path);
 		sub = submodule_from_ce(ce);
@@ -365,7 +365,7 @@ static int write_entry(struct cache_entry *ce,
 		break;
 
 	default:
-		return error("unknown file mode for %s in index", path);
+		return error("unknown file mode for %s in index", ce->name);
 	}
 
 finish:
diff --git a/t/t2006-checkout-index-basic.sh b/t/t2006-checkout-index-basic.sh
index 8e181dbf01..d0d7c3f71c 100755
--- a/t/t2006-checkout-index-basic.sh
+++ b/t/t2006-checkout-index-basic.sh
@@ -32,4 +32,27 @@ test_expect_success 'checkout-index reports errors (stdin)' '
 	test_i18ngrep not.in.the.cache stderr
 '
 
+test_expect_success 'checkout-index --temp correctly reports error on missing blobs' '
+	test_when_finished git reset --hard &&
+	missing_blob=$(git hash-object --stdin </dev/null) &&
+	cat >objs <<-EOF &&
+	100644 $missing_blob	file
+	120000 $missing_blob	symlink
+	EOF
+	git update-index --index-info <objs &&
+
+	test_must_fail git checkout-index --temp symlink file 2>stderr &&
+	test_i18ngrep "unable to read sha1 file of file ($missing_blob)" stderr &&
+	test_i18ngrep "unable to read sha1 file of symlink ($missing_blob)" stderr
+'
+
+test_expect_success 'checkout-index --temp correctly reports error for submodules' '
+	git init sub &&
+	test_commit -C sub file &&
+	git submodule add ./sub &&
+	git commit -m sub &&
+	test_must_fail git checkout-index --temp sub 2>stderr &&
+	test_i18ngrep "cannot create temporary submodule sub" stderr
+'
+
 test_done

-- 
2.29.2

Junio C Hamano· Feb 16, 2021, 02:26 UTC · re: Matheus Tavares · lore

Re: [PATCH v2 1/2] write_entry(): fix misuses of `path` in error messages

Matheus Tavares <matheus.bernardino@usp.br> writes:

Show 8 quoted lines

> +test_expect_success 'checkout-index --temp correctly reports error on missing blobs' '
> +	test_when_finished git reset --hard &&
> +	missing_blob=$(git hash-object --stdin </dev/null) &&
> +	cat >objs <<-EOF &&
> +	100644 $missing_blob	file
> +	120000 $missing_blob	symlink
> +	EOF
> +	git update-index --index-info <objs &&

Does this depend on the fact that nobody created a blob with 0-byte length in the test repository before this test? It feels a bit brittle to me, but as long as future test writers are made aware of that they must not "git add" an empty file, this is probably OK.

But it may be more robust to do something like

	missing_blob=$(echo no such blob here | git hash-object --stdin)

perhaps?  I dunno.

Show 15 quoted lines

> +	test_must_fail git checkout-index --temp symlink file 2>stderr &&
> +	test_i18ngrep "unable to read sha1 file of file ($missing_blob)" stderr &&
> +	test_i18ngrep "unable to read sha1 file of symlink ($missing_blob)" stderr
> +'
> +
> +test_expect_success 'checkout-index --temp correctly reports error for submodules' '
> +	git init sub &&
> +	test_commit -C sub file &&
> +	git submodule add ./sub &&
> +	git commit -m sub &&
> +	test_must_fail git checkout-index --temp sub 2>stderr &&
> +	test_i18ngrep "cannot create temporary submodule sub" stderr
> +'
> +
>  test_done

Thanks.

Matheus Tavares· Feb 15, 2021, 18:24 UTC · re: Matheus Tavares · lore

[PATCH v2 2/2] checkout-index: omit entries with no tempname from --temp output

$ missing_blob=$(git hash-object --stdin </dev/null)
$ git update-index --add --cacheinfo 120000,$missing_blob,foo
$ git checkout-index --temp foo
error: unable to read sha1 file of foo (e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)
        foo

The 'TAB foo' line is not much useful and it might break scripts that expect the 'tempname TAB foo' output. So let's omit such entries from the stdout list (but leaving the error message on stderr).

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 builtin/checkout-index.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

Show changes to builtin/checkout-index.c +24 −11

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 4bbfc92dce..023e49e271 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -23,22 +23,35 @@ static struct checkout state = CHECKOUT_INIT;
 static void write_tempfile_record(const char *name, const char *prefix)
 {
 	int i;
+	int have_tempname = 0;
 
 	if (CHECKOUT_ALL == checkout_stage) {
-		for (i = 1; i < 4; i++) {
-			if (i > 1)
-				putchar(' ');
-			if (topath[i][0])
-				fputs(topath[i], stdout);
-			else
-				putchar('.');
+		for (i = 1; i < 4; i++)
+			if (topath[i][0]) {
+				have_tempname = 1;
+				break;
+			}
+
+		if (have_tempname) {
+			for (i = 1; i < 4; i++) {
+				if (i > 1)
+					putchar(' ');
+				if (topath[i][0])
+					fputs(topath[i], stdout);
+				else
+					putchar('.');
+			}
 		}
-	} else
+	} else if (topath[checkout_stage][0]) {
+		have_tempname = 1;
 		fputs(topath[checkout_stage], stdout);
+	}
 
-	putchar('\t');
-	write_name_quoted_relative(name, prefix, stdout,
-				   nul_term_line ? '\0' : '\n');
+	if (have_tempname) {
+		putchar('\t');
+		write_name_quoted_relative(name, prefix, stdout,
+					   nul_term_line ? '\0' : '\n');
+	}
 
 	for (i = 0; i < 4; i++) {
 		topath[i][0] = 0;

-- 
2.29.2

Matheus Tavares· Feb 16, 2021, 14:06 UTC · re: Matheus Tavares · lore

[PATCH v3 0/2] checkout-index: some cleanups to --temp and --prefix outputs

Change since v2: Make the test added by patch 1 more robust by not depending on the absence of 0-length blobs in the test repository.

Matheus Tavares (2):
  write_entry(): fix misuses of `path` in error messages
  checkout-index: omit entries with no tempname from --temp output

 builtin/checkout-index.c        | 35 ++++++++++++++++++++++-----------
 entry.c                         |  8 ++++----
 t/t2006-checkout-index-basic.sh | 23 ++++++++++++++++++++++
 3 files changed, 51 insertions(+), 15 deletions(-)

Range-diff against v2:
1:  bdda5f99d0 ! 1:  41c166d380 write_entry(): fix misuses of `path` in error messages
    @@ t/t2006-checkout-index-basic.sh: test_expect_success 'checkout-index reports err
      
     +test_expect_success 'checkout-index --temp correctly reports error on missing blobs' '
     +	test_when_finished git reset --hard &&
    -+	missing_blob=$(git hash-object --stdin </dev/null) &&
    ++	missing_blob=$(echo "no such blob here" | git hash-object --stdin) &&
     +	cat >objs <<-EOF &&
     +	100644 $missing_blob	file
     +	120000 $missing_blob	symlink
2:  6ece1947c1 = 2:  8dec184326 checkout-index: omit entries with no tempname from --temp output

-- 
2.29.2

Matheus Tavares· Feb 16, 2021, 14:06 UTC · re: Matheus Tavares · lore

[PATCH v3 2/2] checkout-index: omit entries with no tempname from --temp output

$ missing_blob=$(git hash-object --stdin </dev/null)
$ git update-index --add --cacheinfo 120000,$missing_blob,foo
$ git checkout-index --temp foo
error: unable to read sha1 file of foo (e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)
        foo

The 'TAB foo' line is not much useful and it might break scripts that expect the 'tempname TAB foo' output. So let's omit such entries from the stdout list (but leaving the error message on stderr).

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 builtin/checkout-index.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

Show changes to builtin/checkout-index.c +24 −11

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 4bbfc92dce..023e49e271 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -23,22 +23,35 @@ static struct checkout state = CHECKOUT_INIT;
 static void write_tempfile_record(const char *name, const char *prefix)
 {
 	int i;
+	int have_tempname = 0;
 
 	if (CHECKOUT_ALL == checkout_stage) {
-		for (i = 1; i < 4; i++) {
-			if (i > 1)
-				putchar(' ');
-			if (topath[i][0])
-				fputs(topath[i], stdout);
-			else
-				putchar('.');
+		for (i = 1; i < 4; i++)
+			if (topath[i][0]) {
+				have_tempname = 1;
+				break;
+			}
+
+		if (have_tempname) {
+			for (i = 1; i < 4; i++) {
+				if (i > 1)
+					putchar(' ');
+				if (topath[i][0])
+					fputs(topath[i], stdout);
+				else
+					putchar('.');
+			}
 		}
-	} else
+	} else if (topath[checkout_stage][0]) {
+		have_tempname = 1;
 		fputs(topath[checkout_stage], stdout);
+	}
 
-	putchar('\t');
-	write_name_quoted_relative(name, prefix, stdout,
-				   nul_term_line ? '\0' : '\n');
+	if (have_tempname) {
+		putchar('\t');
+		write_name_quoted_relative(name, prefix, stdout,
+					   nul_term_line ? '\0' : '\n');
+	}
 
 	for (i = 0; i < 4; i++) {
 		topath[i][0] = 0;

-- 
2.29.2

Matheus Tavares· Feb 16, 2021, 14:06 UTC · re: Matheus Tavares · lore

[PATCH v3 1/2] write_entry(): fix misuses of `path` in error messages

The variables `path` and `ce->name`, at write_entry(), usually have the same contents, but that's not the case when using a checkout prefix or writing to a tempfile. (In fact, `path` will be either empty or dirty when writing to a tempfile.) Therefore, these variables cannot be used interchangeably. In this sense, fix wrong uses of `path` in error messages where it should really be `ce->name`, and add some regression tests. (Note: there doesn't seem to be any misuse in the other way around.)

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 entry.c                         |  8 ++++----
 t/t2006-checkout-index-basic.sh | 23 +++++++++++++++++++++++
 2 files changed, 27 insertions(+), 4 deletions(-)

Show changes to 2 files +27 −4

entry.c, t/t2006-checkout-index-basic.sh

diff --git a/entry.c b/entry.c
index a0532f1f00..7b9f43716f 100644
--- a/entry.c
+++ b/entry.c
@@ -282,7 +282,7 @@ static int write_entry(struct cache_entry *ce,
 		new_blob = read_blob_entry(ce, &size);
 		if (!new_blob)
 			return error("unable to read sha1 file of %s (%s)",
-				     path, oid_to_hex(&ce->oid));
+				     ce->name, oid_to_hex(&ce->oid));
 
 		/*
 		 * We can't make a real symlink; write out a regular file entry
@@ -309,7 +309,7 @@ static int write_entry(struct cache_entry *ce,
 			new_blob = read_blob_entry(ce, &size);
 			if (!new_blob)
 				return error("unable to read sha1 file of %s (%s)",
-					     path, oid_to_hex(&ce->oid));
+					     ce->name, oid_to_hex(&ce->oid));
 		}
 
 		/*
@@ -354,7 +354,7 @@ static int write_entry(struct cache_entry *ce,
 
 	case S_IFGITLINK:
 		if (to_tempfile)
-			return error("cannot create temporary submodule %s", path);
+			return error("cannot create temporary submodule %s", ce->name);
 		if (mkdir(path, 0777) < 0)
 			return error("cannot create submodule directory %s", path);
 		sub = submodule_from_ce(ce);
@@ -365,7 +365,7 @@ static int write_entry(struct cache_entry *ce,
 		break;
 
 	default:
-		return error("unknown file mode for %s in index", path);
+		return error("unknown file mode for %s in index", ce->name);
 	}
 
 finish:
diff --git a/t/t2006-checkout-index-basic.sh b/t/t2006-checkout-index-basic.sh
index 8e181dbf01..7ff3edab05 100755
--- a/t/t2006-checkout-index-basic.sh
+++ b/t/t2006-checkout-index-basic.sh
@@ -32,4 +32,27 @@ test_expect_success 'checkout-index reports errors (stdin)' '
 	test_i18ngrep not.in.the.cache stderr
 '
 
+test_expect_success 'checkout-index --temp correctly reports error on missing blobs' '
+	test_when_finished git reset --hard &&
+	missing_blob=$(echo "no such blob here" | git hash-object --stdin) &&
+	cat >objs <<-EOF &&
+	100644 $missing_blob	file
+	120000 $missing_blob	symlink
+	EOF
+	git update-index --index-info <objs &&
+
+	test_must_fail git checkout-index --temp symlink file 2>stderr &&
+	test_i18ngrep "unable to read sha1 file of file ($missing_blob)" stderr &&
+	test_i18ngrep "unable to read sha1 file of symlink ($missing_blob)" stderr
+'
+
+test_expect_success 'checkout-index --temp correctly reports error for submodules' '
+	git init sub &&
+	test_commit -C sub file &&
+	git submodule add ./sub &&
+	git commit -m sub &&
+	test_must_fail git checkout-index --temp sub 2>stderr &&
+	test_i18ngrep "cannot create temporary submodule sub" stderr
+'
+
 test_done

-- 
2.29.2