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

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

[Matheus Tavares]

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

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

[Matheus Tavares]

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(-)

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

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

[Matheus Tavares]

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(-)

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

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

[Junio C Hamano]

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?

> 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:

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

[Junio C Hamano]

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

> 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.

>  	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.

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

[Matheus Tavares Bernardino]

On Tue, Feb 9, 2021 at 6:35 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.

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

[Matheus Tavares Bernardino]

On Tue, Feb 9, 2021 at 6:27 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.)

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

[Junio C Hamano]

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

>> 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.

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

[Matheus Tavares]

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

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

[Matheus Tavares]

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(-)

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

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

[Matheus Tavares]

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 | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

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

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

[Junio C Hamano]

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

> +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.

> +	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.

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

[Matheus Tavares]

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

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

[Matheus Tavares]

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(-)

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

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

[Matheus Tavares]

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 | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

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