* [PATCH 0/2] checkout-index: some cleanups to --temp and --prefix outputs @ 2021-02-08 19:36 Matheus Tavares 2021-02-08 19:36 ` [PATCH 1/2] write_entry(): fix misuses of `path` in error messages Matheus Tavares ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Matheus Tavares @ 2021-02-08 19:36 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] write_entry(): fix misuses of `path` in error messages 2021-02-08 19:36 [PATCH 0/2] checkout-index: some cleanups to --temp and --prefix outputs Matheus Tavares @ 2021-02-08 19:36 ` Matheus Tavares 2021-02-09 21:27 ` Junio C Hamano 2021-02-08 19:36 ` [PATCH 2/2] checkout-index: omit entries with no tempname from --temp output Matheus Tavares 2021-02-15 18:24 ` [PATCH v2 0/2] checkout-index: some cleanups to --temp and --prefix outputs Matheus Tavares 2 siblings, 1 reply; 15+ messages in thread From: Matheus Tavares @ 2021-02-08 19:36 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] write_entry(): fix misuses of `path` in error messages 2021-02-08 19:36 ` [PATCH 1/2] write_entry(): fix misuses of `path` in error messages Matheus Tavares @ 2021-02-09 21:27 ` Junio C Hamano 2021-02-09 23:59 ` Matheus Tavares Bernardino 0 siblings, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2021-02-09 21:27 UTC (permalink / raw) To: Matheus Tavares; +Cc: git 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: ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] write_entry(): fix misuses of `path` in error messages 2021-02-09 21:27 ` Junio C Hamano @ 2021-02-09 23:59 ` Matheus Tavares Bernardino 0 siblings, 0 replies; 15+ messages in thread From: Matheus Tavares Bernardino @ 2021-02-09 23:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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.) ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] checkout-index: omit entries with no tempname from --temp output 2021-02-08 19:36 [PATCH 0/2] checkout-index: some cleanups to --temp and --prefix outputs Matheus Tavares 2021-02-08 19:36 ` [PATCH 1/2] write_entry(): fix misuses of `path` in error messages Matheus Tavares @ 2021-02-08 19:36 ` Matheus Tavares 2021-02-09 21:35 ` Junio C Hamano 2021-02-15 18:24 ` [PATCH v2 0/2] checkout-index: some cleanups to --temp and --prefix outputs Matheus Tavares 2 siblings, 1 reply; 15+ messages in thread From: Matheus Tavares @ 2021-02-08 19:36 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] checkout-index: omit entries with no tempname from --temp output 2021-02-08 19:36 ` [PATCH 2/2] checkout-index: omit entries with no tempname from --temp output Matheus Tavares @ 2021-02-09 21:35 ` Junio C Hamano 2021-02-09 21:57 ` Matheus Tavares Bernardino 0 siblings, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2021-02-09 21:35 UTC (permalink / raw) To: Matheus Tavares; +Cc: git 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] checkout-index: omit entries with no tempname from --temp output 2021-02-09 21:35 ` Junio C Hamano @ 2021-02-09 21:57 ` Matheus Tavares Bernardino 2021-02-10 1:07 ` Junio C Hamano 0 siblings, 1 reply; 15+ messages in thread From: Matheus Tavares Bernardino @ 2021-02-09 21:57 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] checkout-index: omit entries with no tempname from --temp output 2021-02-09 21:57 ` Matheus Tavares Bernardino @ 2021-02-10 1:07 ` Junio C Hamano 0 siblings, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2021-02-10 1:07 UTC (permalink / raw) To: Matheus Tavares Bernardino; +Cc: git 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 0/2] checkout-index: some cleanups to --temp and --prefix outputs 2021-02-08 19:36 [PATCH 0/2] checkout-index: some cleanups to --temp and --prefix outputs Matheus Tavares 2021-02-08 19:36 ` [PATCH 1/2] write_entry(): fix misuses of `path` in error messages Matheus Tavares 2021-02-08 19:36 ` [PATCH 2/2] checkout-index: omit entries with no tempname from --temp output Matheus Tavares @ 2021-02-15 18:24 ` Matheus Tavares 2021-02-15 18:24 ` [PATCH v2 1/2] write_entry(): fix misuses of `path` in error messages Matheus Tavares ` (2 more replies) 2 siblings, 3 replies; 15+ messages in thread From: Matheus Tavares @ 2021-02-15 18:24 UTC (permalink / raw) To: git; +Cc: gitster 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/2] write_entry(): fix misuses of `path` in error messages 2021-02-15 18:24 ` [PATCH v2 0/2] checkout-index: some cleanups to --temp and --prefix outputs Matheus Tavares @ 2021-02-15 18:24 ` Matheus Tavares 2021-02-16 2:26 ` Junio C Hamano 2021-02-15 18:24 ` [PATCH v2 2/2] checkout-index: omit entries with no tempname from --temp output Matheus Tavares 2021-02-16 14:06 ` [PATCH v3 0/2] checkout-index: some cleanups to --temp and --prefix outputs Matheus Tavares 2 siblings, 1 reply; 15+ messages in thread From: Matheus Tavares @ 2021-02-15 18:24 UTC (permalink / raw) To: git; +Cc: gitster 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] write_entry(): fix misuses of `path` in error messages 2021-02-15 18:24 ` [PATCH v2 1/2] write_entry(): fix misuses of `path` in error messages Matheus Tavares @ 2021-02-16 2:26 ` Junio C Hamano 0 siblings, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2021-02-16 2:26 UTC (permalink / raw) To: Matheus Tavares; +Cc: git 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/2] checkout-index: omit entries with no tempname from --temp output 2021-02-15 18:24 ` [PATCH v2 0/2] checkout-index: some cleanups to --temp and --prefix outputs Matheus Tavares 2021-02-15 18:24 ` [PATCH v2 1/2] write_entry(): fix misuses of `path` in error messages Matheus Tavares @ 2021-02-15 18:24 ` Matheus Tavares 2021-02-16 14:06 ` [PATCH v3 0/2] checkout-index: some cleanups to --temp and --prefix outputs Matheus Tavares 2 siblings, 0 replies; 15+ messages in thread From: Matheus Tavares @ 2021-02-15 18:24 UTC (permalink / raw) To: git; +Cc: gitster 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 0/2] checkout-index: some cleanups to --temp and --prefix outputs 2021-02-15 18:24 ` [PATCH v2 0/2] checkout-index: some cleanups to --temp and --prefix outputs Matheus Tavares 2021-02-15 18:24 ` [PATCH v2 1/2] write_entry(): fix misuses of `path` in error messages Matheus Tavares 2021-02-15 18:24 ` [PATCH v2 2/2] checkout-index: omit entries with no tempname from --temp output Matheus Tavares @ 2021-02-16 14:06 ` Matheus Tavares 2021-02-16 14:06 ` [PATCH v3 1/2] write_entry(): fix misuses of `path` in error messages Matheus Tavares 2021-02-16 14:06 ` [PATCH v3 2/2] checkout-index: omit entries with no tempname from --temp output Matheus Tavares 2 siblings, 2 replies; 15+ messages in thread From: Matheus Tavares @ 2021-02-16 14:06 UTC (permalink / raw) To: git; +Cc: gitster 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 1/2] write_entry(): fix misuses of `path` in error messages 2021-02-16 14:06 ` [PATCH v3 0/2] checkout-index: some cleanups to --temp and --prefix outputs Matheus Tavares @ 2021-02-16 14:06 ` Matheus Tavares 2021-02-16 14:06 ` [PATCH v3 2/2] checkout-index: omit entries with no tempname from --temp output Matheus Tavares 1 sibling, 0 replies; 15+ messages in thread From: Matheus Tavares @ 2021-02-16 14:06 UTC (permalink / raw) To: git; +Cc: gitster 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 2/2] checkout-index: omit entries with no tempname from --temp output 2021-02-16 14:06 ` [PATCH v3 0/2] checkout-index: some cleanups to --temp and --prefix outputs Matheus Tavares 2021-02-16 14:06 ` [PATCH v3 1/2] write_entry(): fix misuses of `path` in error messages Matheus Tavares @ 2021-02-16 14:06 ` Matheus Tavares 1 sibling, 0 replies; 15+ messages in thread From: Matheus Tavares @ 2021-02-16 14:06 UTC (permalink / raw) To: git; +Cc: gitster 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-02-16 14:07 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-08 19:36 [PATCH 0/2] checkout-index: some cleanups to --temp and --prefix outputs Matheus Tavares 2021-02-08 19:36 ` [PATCH 1/2] write_entry(): fix misuses of `path` in error messages Matheus Tavares 2021-02-09 21:27 ` Junio C Hamano 2021-02-09 23:59 ` Matheus Tavares Bernardino 2021-02-08 19:36 ` [PATCH 2/2] checkout-index: omit entries with no tempname from --temp output Matheus Tavares 2021-02-09 21:35 ` Junio C Hamano 2021-02-09 21:57 ` Matheus Tavares Bernardino 2021-02-10 1:07 ` Junio C Hamano 2021-02-15 18:24 ` [PATCH v2 0/2] checkout-index: some cleanups to --temp and --prefix outputs Matheus Tavares 2021-02-15 18:24 ` [PATCH v2 1/2] write_entry(): fix misuses of `path` in error messages Matheus Tavares 2021-02-16 2:26 ` Junio C Hamano 2021-02-15 18:24 ` [PATCH v2 2/2] checkout-index: omit entries with no tempname from --temp output Matheus Tavares 2021-02-16 14:06 ` [PATCH v3 0/2] checkout-index: some cleanups to --temp and --prefix outputs Matheus Tavares 2021-02-16 14:06 ` [PATCH v3 1/2] write_entry(): fix misuses of `path` in error messages Matheus Tavares 2021-02-16 14:06 ` [PATCH v3 2/2] checkout-index: omit entries with no tempname from --temp output Matheus Tavares
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).