{"thread":{"id":"55121","subject":"[PATCH 1/2] write_entry(): fix misuses of `path` in error messages","startedAt":"2021-02-08T19:38:50Z","lastAt":"2021-02-16T14:07:53Z","messageCount":15,"participants":["Matheus Tavares","Junio C Hamano","Matheus Tavares Bernardino"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"416436","messageId":"d52bcad326ece6be2fcf87ca6b72e4ce8212e31f.1612812581.git.matheus.bernardino@usp.br","threadId":"55121","inReplyTo":"cover.1612812581.git.matheus.bernardino@usp.br","subject":"[PATCH 1/2] write_entry(): fix misuses of `path` in error messages","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-02-08T19:36:31Z","receivedAt":"2021-02-08T19:38:50Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"The variables `path` and `ce->name`, at write_entry(), usually have the\nsame contents, but that's not the case when using a checkout prefix or\nwriting to a tempfile. (In fact, `path` will be either empty or dirty\nwhen writing to a tempfile.) Therefore, these variables cannot be used\ninterchangeably. In this sense, fix wrong uses of `path` in error\nmessages where it should really be `ce->name`. (There doesn't seem to be\nany misuse in the other way around.)\n\nSigned-off-by: Matheus Tavares \n---\n entry.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex a0532f1f00..7b9f43716f 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -282,7 +282,7 @@ static int write_entry(struct cache_entry *ce,\n \t\tnew_blob = read_blob_entry(ce, &size);\n \t\tif (!new_blob)\n \t\t\treturn error(\"unable to read sha1 file of %s (%s)\",\n-\t\t\t\t path, oid_to_hex(&ce->oid));\n+\t\t\t\t ce->name, oid_to_hex(&ce->oid));\n \n \t\t/*\n \t\t * We can't make a real symlink; write out a regular file entry\n@@ -309,7 +309,7 @@ static int write_entry(struct cache_entry *ce,\n \t\t\tnew_blob = read_blob_entry(ce, &size);\n \t\t\tif (!new_blob)\n \t\t\t\treturn error(\"unable to read sha1 file of %s (%s)\",\n-\t\t\t\t\t path, oid_to_hex(&ce->oid));\n+\t\t\t\t\t ce->name, oid_to_hex(&ce->oid));\n \t\t}\n \n \t\t/*\n@@ -354,7 +354,7 @@ static int write_entry(struct cache_entry *ce,\n \n \tcase S_IFGITLINK:\n \t\tif (to_tempfile)\n-\t\t\treturn error(\"cannot create temporary submodule %s\", path);\n+\t\t\treturn error(\"cannot create temporary submodule %s\", ce->name);\n \t\tif (mkdir(path, 0777) < 0)\n \t\t\treturn error(\"cannot create submodule directory %s\", path);\n \t\tsub = submodule_from_ce(ce);\n@@ -365,7 +365,7 @@ static int write_entry(struct cache_entry *ce,\n \t\tbreak;\n \n \tdefault:\n-\t\treturn error(\"unknown file mode for %s in index\", path);\n+\t\treturn error(\"unknown file mode for %s in index\", ce->name);\n \t}\n \n finish:\n-- \n2.29.2\n\n"},{"id":"416437","messageId":"cover.1612812581.git.matheus.bernardino@usp.br","threadId":"55121","inReplyTo":null,"subject":"[PATCH 0/2] checkout-index: some cleanups to --temp and --prefix outputs","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-02-08T19:36:30Z","receivedAt":"2021-02-08T19:39:30Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"These are some minor cleanups to the checkout-index output when using\n--temp or --prefix. The first patch fixes a few wrong uses of `path` in\nwrite_entry() when it should be `ce->name`, and the second patch removes\nmalformed entries from the --temp output list.\n\nMatheus Tavares (2):\n write_entry(): fix misuses of `path` in error messages\n checkout-index: omit entries with no tempname from --temp output\n\n builtin/checkout-index.c | 39 ++++++++++++++++++++++++++-------------\n entry.c | 8 ++++----\n 2 files changed, 30 insertions(+), 17 deletions(-)\n\n-- \n2.29.2\n\n"},{"id":"416438","messageId":"1275701345b7e198ec83ad4fdcc2dda6d9775ef3.1612812581.git.matheus.bernardino@usp.br","threadId":"55121","inReplyTo":"cover.1612812581.git.matheus.bernardino@usp.br","subject":"[PATCH 2/2] checkout-index: omit entries with no tempname from --temp output","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-02-08T19:36:32Z","receivedAt":"2021-02-08T19:39:30Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"With --temp (or --stage=all, which implies --temp), checkout-index\nwrites a list to stdout associating temporary file names to the entries'\nnames. But if it fails to write an entry, and the failure happens before\neven assigning a temporary filename to that entry, we get an odd output\nline. This can be seen when trying to check out a symlink whose blob is\nmissing:\n\n$ missing_blob=$(git hash-object --stdin \n---\n builtin/checkout-index.c | 39 ++++++++++++++++++++++++++-------------\n 1 file changed, 26 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex 4bbfc92dce..a9f0f0a225 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -23,25 +23,38 @@ static struct checkout state = CHECKOUT_INIT;\n static void write_tempfile_record(const char *name, const char *prefix)\n {\n \tint i;\n+\tint have_tempname = 0;\n \n \tif (CHECKOUT_ALL == checkout_stage) {\n-\t\tfor (i = 1; i < 4; i++) {\n-\t\t\tif (i > 1)\n-\t\t\t\tputchar(' ');\n-\t\t\tif (topath[i][0])\n-\t\t\t\tfputs(topath[i], stdout);\n-\t\t\telse\n-\t\t\t\tputchar('.');\n+\t\tfor (i = 1; i < 4; i++)\n+\t\t\tif (topath[i][0]) {\n+\t\t\t\thave_tempname = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\n+\t\tif (have_tempname) {\n+\t\t\tfor (i = 1; i < 4; i++) {\n+\t\t\t\tif (i > 1)\n+\t\t\t\t\tputchar(' ');\n+\t\t\t\tif (topath[i][0])\n+\t\t\t\t\tfputs(topath[i], stdout);\n+\t\t\t\telse\n+\t\t\t\t\tputchar('.');\n+\t\t\t}\n \t\t}\n-\t} else\n+\t} else if (topath[checkout_stage][0]) {\n+\t\thave_tempname = 1;\n \t\tfputs(topath[checkout_stage], stdout);\n+\t}\n \n-\tputchar('\\t');\n-\twrite_name_quoted_relative(name, prefix, stdout,\n-\t\t\t\t nul_term_line ? '\\0' : '\\n');\n+\tif (have_tempname) {\n+\t\tputchar('\\t');\n+\t\twrite_name_quoted_relative(name, prefix, stdout,\n+\t\t\t\t\t nul_term_line ? '\\0' : '\\n');\n \n-\tfor (i = 0; i < 4; i++) {\n-\t\ttopath[i][0] = 0;\n+\t\tfor (i = 0; i < 4; i++) {\n+\t\t\ttopath[i][0] = 0;\n+\t\t}\n \t}\n }\n \n-- \n2.29.2\n\n"},{"id":"416564","messageId":"xmqqmtwczzyv.fsf@gitster.c.googlers.com","threadId":"55121","inReplyTo":"1275701345b7e198ec83ad4fdcc2dda6d9775ef3.1612812581.git.matheus.bernardino@usp.br","subject":"Re: [PATCH 2/2] checkout-index: omit entries with no tempname from --temp output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-09T21:35:36Z","receivedAt":"2021-02-09T22:24:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares writes:\n\n> With --temp (or --stage=all, which implies --temp), checkout-index\n> writes a list to stdout associating temporary file names to the entries'\n> names. But if it fails to write an entry, and the failure happens before\n> even assigning a temporary filename to that entry, we get an odd output\n> line. This can be seen when trying to check out a symlink whose blob is\n> missing:\n>\n> $ missing_blob=$(git hash-object --stdin $ git update-index --add --cacheinfo 120000,$missing_blob,foo\n> $ git checkout-index --temp foo\n> error: unable to read sha1 file of foo (e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)\n> foo\n>\n> The 'TAB foo' line is not much useful and it might break scripts that\n> expect the 'tempname TAB foo' output. So let's omit such entries from\n> the stdout list (but leaving the error message on stderr).\n\nMakes quite a lot of sense.\n\n> \tif (CHECKOUT_ALL == checkout_stage) {\n> -\t\tfor (i = 1; i < 4; i++) {\n> -\t\t\tif (i > 1)\n> -\t\t\t\tputchar(' ');\n> -\t\t\tif (topath[i][0])\n> -\t\t\t\tfputs(topath[i], stdout);\n> -\t\t\telse\n> -\t\t\t\tputchar('.');\n> +\t\tfor (i = 1; i < 4; i++)\n> +\t\t\tif (topath[i][0]) {\n> +\t\t\t\thave_tempname = 1;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\tif (have_tempname) {\n> +\t\t\tfor (i = 1; i < 4; i++) {\n> +\t\t\t\tif (i > 1)\n> +\t\t\t\t\tputchar(' ');\n> +\t\t\t\tif (topath[i][0])\n> +\t\t\t\t\tfputs(topath[i], stdout);\n> +\t\t\t\telse\n> +\t\t\t\t\tputchar('.');\n> +\t\t\t}\n> \t\t}\n> -\t} else\n> +\t} else if (topath[checkout_stage][0]) {\n> +\t\thave_tempname = 1;\n> \t\tfputs(topath[checkout_stage], stdout);\n> +\t}\n> \n> -\tputchar('\\t');\n> -\twrite_name_quoted_relative(name, prefix, stdout,\n> -\t\t\t\t nul_term_line ? '\\0' : '\\n');\n> +\tif (have_tempname) {\n> +\t\tputchar('\\t');\n> +\t\twrite_name_quoted_relative(name, prefix, stdout,\n> +\t\t\t\t\t nul_term_line ? '\\0' : '\\n');\n> \n> -\tfor (i = 0; i < 4; i++) {\n> -\t\ttopath[i][0] = 0;\n> +\t\tfor (i = 0; i < 4; i++) {\n> +\t\t\ttopath[i][0] = 0;\n> +\t\t}\n> \t}\n\nHmph, is topath[][] array used after this function gets called and\nin what way? Whether have_tempname is true or not, wouldn't we want\nto clear it?\n\nThanks.\n\n"},{"id":"416573","messageId":"CAHd-oW4ddei-bRZkQUSpMevyZYgOBi2T6uQPzJ_XscTmfxhWtw@mail.gmail.com","threadId":"55121","inReplyTo":"xmqqmtwczzyv.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] checkout-index: omit entries with no tempname from --temp output","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-02-09T21:57:09Z","receivedAt":"2021-02-09T22:59:13Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Tue, Feb 9, 2021 at 6:35 PM Junio C Hamano wrote:\n>\n> Matheus Tavares writes:\n>\n> > if (CHECKOUT_ALL == checkout_stage) {\n> > - for (i = 1; i < 4; i++) {\n> > - if (i > 1)\n> > - putchar(' ');\n> > - if (topath[i][0])\n> > - fputs(topath[i], stdout);\n> > - else\n> > - putchar('.');\n> > + for (i = 1; i < 4; i++)\n> > + if (topath[i][0]) {\n> > + have_tempname = 1;\n> > + break;\n> > + }\n> > +\n> > + if (have_tempname) {\n> > + for (i = 1; i < 4; i++) {\n> > + if (i > 1)\n> > + putchar(' ');\n> > + if (topath[i][0])\n> > + fputs(topath[i], stdout);\n> > + else\n> > + putchar('.');\n> > + }\n> > }\n> > - } else\n> > + } else if (topath[checkout_stage][0]) {\n> > + have_tempname = 1;\n> > fputs(topath[checkout_stage], stdout);\n> > + }\n> >\n> > - putchar('\\t');\n> > - write_name_quoted_relative(name, prefix, stdout,\n> > - nul_term_line ? '\\0' : '\\n');\n> > + if (have_tempname) {\n> > + putchar('\\t');\n> > + write_name_quoted_relative(name, prefix, stdout,\n> > + nul_term_line ? '\\0' : '\\n');\n> >\n> > - for (i = 0; i < 4; i++) {\n> > - topath[i][0] = 0;\n> > + for (i = 0; i < 4; i++) {\n> > + topath[i][0] = 0;\n> > + }\n> > }\n>\n> Hmph, is topath[][] array used after this function gets called and\n> in what way? Whether have_tempname is true or not, wouldn't we want\n> to clear it?\n\nYeah, topath[][] can be reused in the next checkout_entry() call. But\nif have_tempname is false, the positions that are going to be used\nagain (either checkout_stage or 1, 2, and 3, if checkout_stage ==\nCHECKOUT_ALL) will be already empty. So I think we only need to clear\ntopath[][] when have_tempname is false.\n"},{"id":"416587","messageId":"xmqqr1lpyls8.fsf@gitster.c.googlers.com","threadId":"55121","inReplyTo":"d52bcad326ece6be2fcf87ca6b72e4ce8212e31f.1612812581.git.matheus.bernardino@usp.br","subject":"Re: [PATCH 1/2] write_entry(): fix misuses of `path` in error messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-09T21:27:19Z","receivedAt":"2021-02-09T23:13:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares writes:\n\n> The variables `path` and `ce->name`, at write_entry(), usually have the\n> same contents, but that's not the case when using a checkout prefix or\n> writing to a tempfile. (In fact, `path` will be either empty or dirty\n> when writing to a tempfile.) Therefore, these variables cannot be used\n> interchangeably. In this sense, fix wrong uses of `path` in error\n> messages where it should really be `ce->name`. (There doesn't seem to be\n> any misuse in the other way around.)\n\nSounds reasonable. Don't we want to protect this fix with tests?\n\n> Signed-off-by: Matheus Tavares \n> ---\n> entry.c | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/entry.c b/entry.c\n> index a0532f1f00..7b9f43716f 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -282,7 +282,7 @@ static int write_entry(struct cache_entry *ce,\n> \t\tnew_blob = read_blob_entry(ce, &size);\n> \t\tif (!new_blob)\n> \t\t\treturn error(\"unable to read sha1 file of %s (%s)\",\n> -\t\t\t\t path, oid_to_hex(&ce->oid));\n> +\t\t\t\t ce->name, oid_to_hex(&ce->oid));\n> \n> \t\t/*\n> \t\t * We can't make a real symlink; write out a regular file entry\n> @@ -309,7 +309,7 @@ static int write_entry(struct cache_entry *ce,\n> \t\t\tnew_blob = read_blob_entry(ce, &size);\n> \t\t\tif (!new_blob)\n> \t\t\t\treturn error(\"unable to read sha1 file of %s (%s)\",\n> -\t\t\t\t\t path, oid_to_hex(&ce->oid));\n> +\t\t\t\t\t ce->name, oid_to_hex(&ce->oid));\n> \t\t}\n> \n> \t\t/*\n> @@ -354,7 +354,7 @@ static int write_entry(struct cache_entry *ce,\n> \n> \tcase S_IFGITLINK:\n> \t\tif (to_tempfile)\n> -\t\t\treturn error(\"cannot create temporary submodule %s\", path);\n> +\t\t\treturn error(\"cannot create temporary submodule %s\", ce->name);\n> \t\tif (mkdir(path, 0777) < 0)\n> \t\t\treturn error(\"cannot create submodule directory %s\", path);\n> \t\tsub = submodule_from_ce(ce);\n> @@ -365,7 +365,7 @@ static int write_entry(struct cache_entry *ce,\n> \t\tbreak;\n> \n> \tdefault:\n> -\t\treturn error(\"unknown file mode for %s in index\", path);\n> +\t\treturn error(\"unknown file mode for %s in index\", ce->name);\n> \t}\n> \n> finish:\n"},{"id":"416594","messageId":"CAHd-oW6CopkLyJTCErx9AYMw5YuhpUYhc7N4BH4sJKvy-7rvdA@mail.gmail.com","threadId":"55121","inReplyTo":"xmqqr1lpyls8.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] write_entry(): fix misuses of `path` in error messages","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-02-09T23:59:51Z","receivedAt":"2021-02-10T00:12:19Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Tue, Feb 9, 2021 at 6:27 PM Junio C Hamano wrote:\n>\n> Matheus Tavares writes:\n>\n> > The variables `path` and `ce->name`, at write_entry(), usually have the\n> > same contents, but that's not the case when using a checkout prefix or\n> > writing to a tempfile. (In fact, `path` will be either empty or dirty\n> > when writing to a tempfile.) Therefore, these variables cannot be used\n> > interchangeably. In this sense, fix wrong uses of `path` in error\n> > messages where it should really be `ce->name`. (There doesn't seem to be\n> > any misuse in the other way around.)\n>\n> Sounds reasonable. Don't we want to protect this fix with tests?\n\nYeah, good idea. I will add a couple tests to check the error message\non missing blobs and when trying to write a submodule to a tempfile.\nThis should cover 3 out of the 4 error() calls changed in this patch.\n(The last one would be when ce->mode is unknown. I'm not sure if/how I\ncan trigger that case.)\n"},{"id":"416598","messageId":"xmqqa6scyblz.fsf@gitster.c.googlers.com","threadId":"55121","inReplyTo":"CAHd-oW4ddei-bRZkQUSpMevyZYgOBi2T6uQPzJ_XscTmfxhWtw@mail.gmail.com","subject":"Re: [PATCH 2/2] checkout-index: omit entries with no tempname from --temp output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-10T01:07:04Z","receivedAt":"2021-02-10T01:09:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares Bernardino writes:\n\n>> Hmph, is topath[][] array used after this function gets called and\n>> in what way? Whether have_tempname is true or not, wouldn't we want\n>> to clear it?\n>\n> Yeah, topath[][] can be reused in the next checkout_entry() call. But\n> if have_tempname is false, the positions that are going to be used\n> again (either checkout_stage or 1, 2, and 3, if checkout_stage ==\n> CHECKOUT_ALL) will be already empty. So I think we only need to clear\n> topath[][] when have_tempname is false.\n\nIf so, clearing them unconditionally like the original code before\nthe introduction of have_tempname variable would be easier on\nreaders, as they won't be forced to reason about when to and when\nnot to clear these strings---figure out if the reason why we do not\nalways clear is because (1) we have info that we do not want to lose\nif (!have_tempname), or (2) we know there is nothing to be cleared\nif (!have_tempname).\n\nThanks.\n"},{"id":"416953","messageId":"cover.1613411136.git.matheus.bernardino@usp.br","threadId":"55121","inReplyTo":"cover.1612812581.git.matheus.bernardino@usp.br","subject":"[PATCH v2 0/2] checkout-index: some cleanups to --temp and --prefix outputs","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-02-15T18:24:11Z","receivedAt":"2021-02-15T18:25:06Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Changes since v1:\n- Added regression tests in first patch.\n- Left the `topath[]` cleanup outside the if block on patch 2, to make\n it easier to read.\n\nMatheus Tavares (2):\n write_entry(): fix misuses of `path` in error messages\n checkout-index: omit entries with no tempname from --temp output\n\n builtin/checkout-index.c | 35 ++++++++++++++++++++++-----------\n entry.c | 8 ++++----\n t/t2006-checkout-index-basic.sh | 23 ++++++++++++++++++++++\n 3 files changed, 51 insertions(+), 15 deletions(-)\n\nRange-diff against v1:\n1: d52bcad326 ! 1: bdda5f99d0 write_entry(): fix misuses of `path` in error messages\n @@ Commit message\n writing to a tempfile. (In fact, `path` will be either empty or dirty\n when writing to a tempfile.) Therefore, these variables cannot be used\n interchangeably. In this sense, fix wrong uses of `path` in error\n - messages where it should really be `ce->name`. (There doesn't seem to be\n - any misuse in the other way around.)\n + messages where it should really be `ce->name`, and add some regression\n + tests. (Note: there doesn't seem to be any misuse in the other way\n + around.)\n \n Signed-off-by: Matheus Tavares \n \n @@ entry.c: static int write_entry(struct cache_entry *ce,\n \t}\n \n finish:\n +\n + ## t/t2006-checkout-index-basic.sh ##\n +@@ t/t2006-checkout-index-basic.sh: test_expect_success 'checkout-index reports errors (stdin)' '\n + \ttest_i18ngrep not.in.the.cache stderr\n + '\n + \n ++test_expect_success 'checkout-index --temp correctly reports error on missing blobs' '\n ++\ttest_when_finished git reset --hard &&\n ++\tmissing_blob=$(git hash-object --stdin objs <<-EOF &&\n ++\t100644 $missing_blob\tfile\n ++\t120000 $missing_blob\tsymlink\n ++\tEOF\n ++\tgit update-index --index-info stderr &&\n ++\ttest_i18ngrep \"unable to read sha1 file of file ($missing_blob)\" stderr &&\n ++\ttest_i18ngrep \"unable to read sha1 file of symlink ($missing_blob)\" stderr\n ++'\n ++\n ++test_expect_success 'checkout-index --temp correctly reports error for submodules' '\n ++\tgit init sub &&\n ++\ttest_commit -C sub file &&\n ++\tgit submodule add ./sub &&\n ++\tgit commit -m sub &&\n ++\ttest_must_fail git checkout-index --temp sub 2>stderr &&\n ++\ttest_i18ngrep \"cannot create temporary submodule sub\" stderr\n ++'\n ++\n + test_done\n2: 1275701345 ! 2: 6ece1947c1 checkout-index: omit entries with no tempname from --temp output\n @@ builtin/checkout-index.c: static struct checkout state = CHECKOUT_INIT;\n +\t\tputchar('\\t');\n +\t\twrite_name_quoted_relative(name, prefix, stdout,\n +\t\t\t\t\t nul_term_line ? '\\0' : '\\n');\n ++\t}\n \n --\tfor (i = 0; i < 4; i++) {\n --\t\ttopath[i][0] = 0;\n -+\t\tfor (i = 0; i < 4; i++) {\n -+\t\t\ttopath[i][0] = 0;\n -+\t\t}\n - \t}\n - }\n - \n + \tfor (i = 0; i < 4; i++) {\n + \t\ttopath[i][0] = 0;\n-- \n2.29.2\n\n"},{"id":"416954","messageId":"bdda5f99d031abad65c296a61b3713f60d22ef16.1613411136.git.matheus.bernardino@usp.br","threadId":"55121","inReplyTo":"cover.1613411136.git.matheus.bernardino@usp.br","subject":"[PATCH v2 1/2] write_entry(): fix misuses of `path` in error messages","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-02-15T18:24:12Z","receivedAt":"2021-02-15T18:25:07Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"The variables `path` and `ce->name`, at write_entry(), usually have the\nsame contents, but that's not the case when using a checkout prefix or\nwriting to a tempfile. (In fact, `path` will be either empty or dirty\nwhen writing to a tempfile.) Therefore, these variables cannot be used\ninterchangeably. In this sense, fix wrong uses of `path` in error\nmessages where it should really be `ce->name`, and add some regression\ntests. (Note: there doesn't seem to be any misuse in the other way\naround.)\n\nSigned-off-by: Matheus Tavares \n---\n entry.c | 8 ++++----\n t/t2006-checkout-index-basic.sh | 23 +++++++++++++++++++++++\n 2 files changed, 27 insertions(+), 4 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex a0532f1f00..7b9f43716f 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -282,7 +282,7 @@ static int write_entry(struct cache_entry *ce,\n \t\tnew_blob = read_blob_entry(ce, &size);\n \t\tif (!new_blob)\n \t\t\treturn error(\"unable to read sha1 file of %s (%s)\",\n-\t\t\t\t path, oid_to_hex(&ce->oid));\n+\t\t\t\t ce->name, oid_to_hex(&ce->oid));\n \n \t\t/*\n \t\t * We can't make a real symlink; write out a regular file entry\n@@ -309,7 +309,7 @@ static int write_entry(struct cache_entry *ce,\n \t\t\tnew_blob = read_blob_entry(ce, &size);\n \t\t\tif (!new_blob)\n \t\t\t\treturn error(\"unable to read sha1 file of %s (%s)\",\n-\t\t\t\t\t path, oid_to_hex(&ce->oid));\n+\t\t\t\t\t ce->name, oid_to_hex(&ce->oid));\n \t\t}\n \n \t\t/*\n@@ -354,7 +354,7 @@ static int write_entry(struct cache_entry *ce,\n \n \tcase S_IFGITLINK:\n \t\tif (to_tempfile)\n-\t\t\treturn error(\"cannot create temporary submodule %s\", path);\n+\t\t\treturn error(\"cannot create temporary submodule %s\", ce->name);\n \t\tif (mkdir(path, 0777) < 0)\n \t\t\treturn error(\"cannot create submodule directory %s\", path);\n \t\tsub = submodule_from_ce(ce);\n@@ -365,7 +365,7 @@ static int write_entry(struct cache_entry *ce,\n \t\tbreak;\n \n \tdefault:\n-\t\treturn error(\"unknown file mode for %s in index\", path);\n+\t\treturn error(\"unknown file mode for %s in index\", ce->name);\n \t}\n \n finish:\ndiff --git a/t/t2006-checkout-index-basic.sh b/t/t2006-checkout-index-basic.sh\nindex 8e181dbf01..d0d7c3f71c 100755\n--- a/t/t2006-checkout-index-basic.sh\n+++ b/t/t2006-checkout-index-basic.sh\n@@ -32,4 +32,27 @@ test_expect_success 'checkout-index reports errors (stdin)' '\n \ttest_i18ngrep not.in.the.cache stderr\n '\n \n+test_expect_success 'checkout-index --temp correctly reports error on missing blobs' '\n+\ttest_when_finished git reset --hard &&\n+\tmissing_blob=$(git hash-object --stdin objs <<-EOF &&\n+\t100644 $missing_blob\tfile\n+\t120000 $missing_blob\tsymlink\n+\tEOF\n+\tgit update-index --index-info stderr &&\n+\ttest_i18ngrep \"unable to read sha1 file of file ($missing_blob)\" stderr &&\n+\ttest_i18ngrep \"unable to read sha1 file of symlink ($missing_blob)\" stderr\n+'\n+\n+test_expect_success 'checkout-index --temp correctly reports error for submodules' '\n+\tgit init sub &&\n+\ttest_commit -C sub file &&\n+\tgit submodule add ./sub &&\n+\tgit commit -m sub &&\n+\ttest_must_fail git checkout-index --temp sub 2>stderr &&\n+\ttest_i18ngrep \"cannot create temporary submodule sub\" stderr\n+'\n+\n test_done\n-- \n2.29.2\n\n"},{"id":"416955","messageId":"6ece1947c1781ee24a3a72f89a1cba39a5ebec37.1613411136.git.matheus.bernardino@usp.br","threadId":"55121","inReplyTo":"cover.1613411136.git.matheus.bernardino@usp.br","subject":"[PATCH v2 2/2] checkout-index: omit entries with no tempname from --temp output","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-02-15T18:24:13Z","receivedAt":"2021-02-15T18:25:08Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"With --temp (or --stage=all, which implies --temp), checkout-index\nwrites a list to stdout associating temporary file names to the entries'\nnames. But if it fails to write an entry, and the failure happens before\neven assigning a temporary filename to that entry, we get an odd output\nline. This can be seen when trying to check out a symlink whose blob is\nmissing:\n\n$ missing_blob=$(git hash-object --stdin \n---\n builtin/checkout-index.c | 35 ++++++++++++++++++++++++-----------\n 1 file changed, 24 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex 4bbfc92dce..023e49e271 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -23,22 +23,35 @@ static struct checkout state = CHECKOUT_INIT;\n static void write_tempfile_record(const char *name, const char *prefix)\n {\n \tint i;\n+\tint have_tempname = 0;\n \n \tif (CHECKOUT_ALL == checkout_stage) {\n-\t\tfor (i = 1; i < 4; i++) {\n-\t\t\tif (i > 1)\n-\t\t\t\tputchar(' ');\n-\t\t\tif (topath[i][0])\n-\t\t\t\tfputs(topath[i], stdout);\n-\t\t\telse\n-\t\t\t\tputchar('.');\n+\t\tfor (i = 1; i < 4; i++)\n+\t\t\tif (topath[i][0]) {\n+\t\t\t\thave_tempname = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\n+\t\tif (have_tempname) {\n+\t\t\tfor (i = 1; i < 4; i++) {\n+\t\t\t\tif (i > 1)\n+\t\t\t\t\tputchar(' ');\n+\t\t\t\tif (topath[i][0])\n+\t\t\t\t\tfputs(topath[i], stdout);\n+\t\t\t\telse\n+\t\t\t\t\tputchar('.');\n+\t\t\t}\n \t\t}\n-\t} else\n+\t} else if (topath[checkout_stage][0]) {\n+\t\thave_tempname = 1;\n \t\tfputs(topath[checkout_stage], stdout);\n+\t}\n \n-\tputchar('\\t');\n-\twrite_name_quoted_relative(name, prefix, stdout,\n-\t\t\t\t nul_term_line ? '\\0' : '\\n');\n+\tif (have_tempname) {\n+\t\tputchar('\\t');\n+\t\twrite_name_quoted_relative(name, prefix, stdout,\n+\t\t\t\t\t nul_term_line ? '\\0' : '\\n');\n+\t}\n \n \tfor (i = 0; i < 4; i++) {\n \t\ttopath[i][0] = 0;\n-- \n2.29.2\n\n"},{"id":"417000","messageId":"xmqqk0r8sq6u.fsf@gitster.c.googlers.com","threadId":"55121","inReplyTo":"bdda5f99d031abad65c296a61b3713f60d22ef16.1613411136.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v2 1/2] write_entry(): fix misuses of `path` in error messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-16T02:26:49Z","receivedAt":"2021-02-16T02:27:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares writes:\n\n> +test_expect_success 'checkout-index --temp correctly reports error on missing blobs' '\n> +\ttest_when_finished git reset --hard &&\n> +\tmissing_blob=$(git hash-object --stdin +\tcat >objs <<-EOF &&\n> +\t100644 $missing_blob\tfile\n> +\t120000 $missing_blob\tsymlink\n> +\tEOF\n> +\tgit update-index --index-info +\ttest_must_fail git checkout-index --temp symlink file 2>stderr &&\n> +\ttest_i18ngrep \"unable to read sha1 file of file ($missing_blob)\" stderr &&\n> +\ttest_i18ngrep \"unable to read sha1 file of symlink ($missing_blob)\" stderr\n> +'\n> +\n> +test_expect_success 'checkout-index --temp correctly reports error for submodules' '\n> +\tgit init sub &&\n> +\ttest_commit -C sub file &&\n> +\tgit submodule add ./sub &&\n> +\tgit commit -m sub &&\n> +\ttest_must_fail git checkout-index --temp sub 2>stderr &&\n> +\ttest_i18ngrep \"cannot create temporary submodule sub\" stderr\n> +'\n> +\n> test_done\n\nThanks.\n"},{"id":"417051","messageId":"cover.1613483833.git.matheus.bernardino@usp.br","threadId":"55121","inReplyTo":"cover.1613411136.git.matheus.bernardino@usp.br","subject":"[PATCH v3 0/2] checkout-index: some cleanups to --temp and --prefix outputs","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-02-16T14:06:50Z","receivedAt":"2021-02-16T14:07:43Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Change since v2:\nMake the test added by patch 1 more robust by not depending on the\nabsence of 0-length blobs in the test repository.\n\nMatheus Tavares (2):\n write_entry(): fix misuses of `path` in error messages\n checkout-index: omit entries with no tempname from --temp output\n\n builtin/checkout-index.c | 35 ++++++++++++++++++++++-----------\n entry.c | 8 ++++----\n t/t2006-checkout-index-basic.sh | 23 ++++++++++++++++++++++\n 3 files changed, 51 insertions(+), 15 deletions(-)\n\nRange-diff against v2:\n1: bdda5f99d0 ! 1: 41c166d380 write_entry(): fix misuses of `path` in error messages\n @@ t/t2006-checkout-index-basic.sh: test_expect_success 'checkout-index reports err\n \n +test_expect_success 'checkout-index --temp correctly reports error on missing blobs' '\n +\ttest_when_finished git reset --hard &&\n -+\tmissing_blob=$(git hash-object --stdin objs <<-EOF &&\n +\t100644 $missing_blob\tfile\n +\t120000 $missing_blob\tsymlink\n2: 6ece1947c1 = 2: 8dec184326 checkout-index: omit entries with no tempname from --temp output\n-- \n2.29.2\n\n"},{"id":"417052","messageId":"8dec184326d1bb4e18975d96f93f536cfd1543fd.1613483833.git.matheus.bernardino@usp.br","threadId":"55121","inReplyTo":"cover.1613483833.git.matheus.bernardino@usp.br","subject":"[PATCH v3 2/2] checkout-index: omit entries with no tempname from --temp output","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-02-16T14:06:52Z","receivedAt":"2021-02-16T14:07:48Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"With --temp (or --stage=all, which implies --temp), checkout-index\nwrites a list to stdout associating temporary file names to the entries'\nnames. But if it fails to write an entry, and the failure happens before\neven assigning a temporary filename to that entry, we get an odd output\nline. This can be seen when trying to check out a symlink whose blob is\nmissing:\n\n$ missing_blob=$(git hash-object --stdin \n---\n builtin/checkout-index.c | 35 ++++++++++++++++++++++++-----------\n 1 file changed, 24 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex 4bbfc92dce..023e49e271 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -23,22 +23,35 @@ static struct checkout state = CHECKOUT_INIT;\n static void write_tempfile_record(const char *name, const char *prefix)\n {\n \tint i;\n+\tint have_tempname = 0;\n \n \tif (CHECKOUT_ALL == checkout_stage) {\n-\t\tfor (i = 1; i < 4; i++) {\n-\t\t\tif (i > 1)\n-\t\t\t\tputchar(' ');\n-\t\t\tif (topath[i][0])\n-\t\t\t\tfputs(topath[i], stdout);\n-\t\t\telse\n-\t\t\t\tputchar('.');\n+\t\tfor (i = 1; i < 4; i++)\n+\t\t\tif (topath[i][0]) {\n+\t\t\t\thave_tempname = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\n+\t\tif (have_tempname) {\n+\t\t\tfor (i = 1; i < 4; i++) {\n+\t\t\t\tif (i > 1)\n+\t\t\t\t\tputchar(' ');\n+\t\t\t\tif (topath[i][0])\n+\t\t\t\t\tfputs(topath[i], stdout);\n+\t\t\t\telse\n+\t\t\t\t\tputchar('.');\n+\t\t\t}\n \t\t}\n-\t} else\n+\t} else if (topath[checkout_stage][0]) {\n+\t\thave_tempname = 1;\n \t\tfputs(topath[checkout_stage], stdout);\n+\t}\n \n-\tputchar('\\t');\n-\twrite_name_quoted_relative(name, prefix, stdout,\n-\t\t\t\t nul_term_line ? '\\0' : '\\n');\n+\tif (have_tempname) {\n+\t\tputchar('\\t');\n+\t\twrite_name_quoted_relative(name, prefix, stdout,\n+\t\t\t\t\t nul_term_line ? '\\0' : '\\n');\n+\t}\n \n \tfor (i = 0; i < 4; i++) {\n \t\ttopath[i][0] = 0;\n-- \n2.29.2\n\n"},{"id":"417053","messageId":"41c166d3806915b372b4b8dfd8c87a299cf6a9c9.1613483833.git.matheus.bernardino@usp.br","threadId":"55121","inReplyTo":"cover.1613483833.git.matheus.bernardino@usp.br","subject":"[PATCH v3 1/2] write_entry(): fix misuses of `path` in error messages","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-02-16T14:06:51Z","receivedAt":"2021-02-16T14:07:53Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"The variables `path` and `ce->name`, at write_entry(), usually have the\nsame contents, but that's not the case when using a checkout prefix or\nwriting to a tempfile. (In fact, `path` will be either empty or dirty\nwhen writing to a tempfile.) Therefore, these variables cannot be used\ninterchangeably. In this sense, fix wrong uses of `path` in error\nmessages where it should really be `ce->name`, and add some regression\ntests. (Note: there doesn't seem to be any misuse in the other way\naround.)\n\nSigned-off-by: Matheus Tavares \n---\n entry.c | 8 ++++----\n t/t2006-checkout-index-basic.sh | 23 +++++++++++++++++++++++\n 2 files changed, 27 insertions(+), 4 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex a0532f1f00..7b9f43716f 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -282,7 +282,7 @@ static int write_entry(struct cache_entry *ce,\n \t\tnew_blob = read_blob_entry(ce, &size);\n \t\tif (!new_blob)\n \t\t\treturn error(\"unable to read sha1 file of %s (%s)\",\n-\t\t\t\t path, oid_to_hex(&ce->oid));\n+\t\t\t\t ce->name, oid_to_hex(&ce->oid));\n \n \t\t/*\n \t\t * We can't make a real symlink; write out a regular file entry\n@@ -309,7 +309,7 @@ static int write_entry(struct cache_entry *ce,\n \t\t\tnew_blob = read_blob_entry(ce, &size);\n \t\t\tif (!new_blob)\n \t\t\t\treturn error(\"unable to read sha1 file of %s (%s)\",\n-\t\t\t\t\t path, oid_to_hex(&ce->oid));\n+\t\t\t\t\t ce->name, oid_to_hex(&ce->oid));\n \t\t}\n \n \t\t/*\n@@ -354,7 +354,7 @@ static int write_entry(struct cache_entry *ce,\n \n \tcase S_IFGITLINK:\n \t\tif (to_tempfile)\n-\t\t\treturn error(\"cannot create temporary submodule %s\", path);\n+\t\t\treturn error(\"cannot create temporary submodule %s\", ce->name);\n \t\tif (mkdir(path, 0777) < 0)\n \t\t\treturn error(\"cannot create submodule directory %s\", path);\n \t\tsub = submodule_from_ce(ce);\n@@ -365,7 +365,7 @@ static int write_entry(struct cache_entry *ce,\n \t\tbreak;\n \n \tdefault:\n-\t\treturn error(\"unknown file mode for %s in index\", path);\n+\t\treturn error(\"unknown file mode for %s in index\", ce->name);\n \t}\n \n finish:\ndiff --git a/t/t2006-checkout-index-basic.sh b/t/t2006-checkout-index-basic.sh\nindex 8e181dbf01..7ff3edab05 100755\n--- a/t/t2006-checkout-index-basic.sh\n+++ b/t/t2006-checkout-index-basic.sh\n@@ -32,4 +32,27 @@ test_expect_success 'checkout-index reports errors (stdin)' '\n \ttest_i18ngrep not.in.the.cache stderr\n '\n \n+test_expect_success 'checkout-index --temp correctly reports error on missing blobs' '\n+\ttest_when_finished git reset --hard &&\n+\tmissing_blob=$(echo \"no such blob here\" | git hash-object --stdin) &&\n+\tcat >objs <<-EOF &&\n+\t100644 $missing_blob\tfile\n+\t120000 $missing_blob\tsymlink\n+\tEOF\n+\tgit update-index --index-info stderr &&\n+\ttest_i18ngrep \"unable to read sha1 file of file ($missing_blob)\" stderr &&\n+\ttest_i18ngrep \"unable to read sha1 file of symlink ($missing_blob)\" stderr\n+'\n+\n+test_expect_success 'checkout-index --temp correctly reports error for submodules' '\n+\tgit init sub &&\n+\ttest_commit -C sub file &&\n+\tgit submodule add ./sub &&\n+\tgit commit -m sub &&\n+\ttest_must_fail git checkout-index --temp sub 2>stderr &&\n+\ttest_i18ngrep \"cannot create temporary submodule sub\" stderr\n+'\n+\n test_done\n-- \n2.29.2\n\n"}]}