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

Matheus Tavares [off-list ref] 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.

On Tue, Feb 9, 2021 at 6:35 PM Junio C Hamano [off-list ref] wrote:

Matheus Tavares [off-list ref] writes:

quoted

      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.