| Message ID | 1275701345b7e198ec83ad4fdcc2dda6d9775ef3.1612812581.git.matheus.bernardino@usp.br (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | checkout-index: some cleanups to --temp and --prefix outputs | expand |
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.
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.
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.
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; + } } }
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(-)