Re: [PATCH v2 1/6] archive: optionally add "virtual" files

["René Scharfe" <l.s.r@web.de>]

Am 08.02.22 um 18:44 schrieb Junio C Hamano:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>>> We could use that option in Git's own Makefile to add the file named
>>>> "version", which contains $GIT_VERSION.  Hmm, but it also contains a
>>>> terminating newline, which would be a bit tricky (but not impossible) to
>>>> add.  Would it make sense to add one automatically if it's missing (e.g.
>>>> with strbuf_complete_line)?  Not sure.
>>>
>>> I do not think it is a good UI to give raw file content from the
>>> command line, which will be usable only for trivial, even single
>>> liner files, and forces people to learn two parallel option, one
>>> for trivial ones and the other for contents with meaningful size.
>>
>> Nevertheless, it is still the most elegant way that I can think of to
>> generate a diagnostic `.zip` file without messing up the very things that
>> are to be diagnosed: the repository and the worktree.
>
> Puzzled.  Are you feeding contents of a .zip file from the command
> line?

Kind of.  Command line arguments are built and handed to write_archive()
in-process.  It's done by patch 3 and extended by 5 and 6.

The number of files is relatively low and they aren't huge, right?
Staging their content in the object database would be messy, but $TMPDIR
might be able to take them with a low impact.  Unless the problem to
diagnose is that this directory is full -- but you don't need a fancy
report for that. :)

Currently there is no easy way to write a temporary file with a chosen
name.  diff.c would benefit from such a thing when running an external
diff program; currently it adds a random prefix.  git archive --add-file
also uses the filename (and discards the directory part).  The patch
below adds a function to create temporary files with a chosen name.
Perhaps it would be useful here as well, instead of the new option?

> I was mostly worried about busting command line argument limit by
> trying to feed too many bytes, as the ceiling is fairly low on some
> platforms.

Command line length limits don't apply to the way scalar uses the new
option.

> Another worry was that when <contents> can have
> arbitrary bytes, with --opt=<path>:<contents> syntax, the input
> becomes ambiguous (i.e. "which colon is the <path> separator?"),
> without some way to escape a colon in the payload.

The first colon is the separator here.

> For a single-liner, --add-file-with-contents=<path>:<contents> would
> be an OK way, and my comment was not a strong objection against this
> new option existing.  It was primarily an objection against changing
> the way to add the 'version' file in our "make dist" procedure to
> use it anyway.
>
> But now I think about it more, I am becoming less happy about it
> existing in the first place.
>
> This will throw another monkey wrench to Konstantin's plan [*] to
> make "git archive" output verifiable with the signature on original
> Git objects, but it is not a new problem ;-)
>
>
> [Reference]
>
> * https://lore.kernel.org/git/20220207213449.ljqjhdx4f45a3lx5@meerkat.local/

I don't see the conflict: If an untracked file is added to an archive
using --add-file, --add-file-with-content, or ZIP or tar then we'd
*want* the verification against a signed commit or tag to fail, no?  A
different signature would be required for the non-tracked parts.

René


--- >8 ---
Subject: [PATCH] tempfile: add mks_tempfile_dt()

Add a function to create a temporary file with a certain name in a
temporary directory created using mkdtemp(3).  Its result is more
sightly than the paths created by mks_tempfile_ts(), which include
a random prefix.  That's useful for files passed to a program that
displays their name, e.g. an external diff tool.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 tempfile.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tempfile.h | 13 +++++++++++
 2 files changed, 76 insertions(+)

diff --git a/tempfile.c b/tempfile.c
index 94aa18f3f7..2024c82691 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -56,6 +56,20 @@

 static VOLATILE_LIST_HEAD(tempfile_list);

+static void remove_template_directory(struct tempfile *tempfile,
+				      int in_signal_handler)
+{
+	if (tempfile->directorylen > 0 &&
+	    tempfile->directorylen < tempfile->filename.len &&
+	    tempfile->filename.buf[tempfile->directorylen] == '/') {
+		strbuf_setlen(&tempfile->filename, tempfile->directorylen);
+		if (in_signal_handler)
+			rmdir(tempfile->filename.buf);
+		else
+			rmdir_or_warn(tempfile->filename.buf);
+	}
+}
+
 static void remove_tempfiles(int in_signal_handler)
 {
 	pid_t me = getpid();
@@ -74,6 +88,7 @@ static void remove_tempfiles(int in_signal_handler)
 			unlink(p->filename.buf);
 		else
 			unlink_or_warn(p->filename.buf);
+		remove_template_directory(p, in_signal_handler);

 		p->active = 0;
 	}
@@ -100,6 +115,7 @@ static struct tempfile *new_tempfile(void)
 	tempfile->owner = 0;
 	INIT_LIST_HEAD(&tempfile->list);
 	strbuf_init(&tempfile->filename, 0);
+	tempfile->directorylen = 0;
 	return tempfile;
 }

@@ -198,6 +214,52 @@ struct tempfile *mks_tempfile_tsm(const char *filename_template, int suffixlen,
 	return tempfile;
 }

+struct tempfile *mks_tempfile_dt(const char *directory_template,
+				 const char *filename)
+{
+	struct tempfile *tempfile;
+	const char *tmpdir;
+	struct strbuf sb = STRBUF_INIT;
+	int fd;
+	size_t directorylen;
+
+	if (!ends_with(directory_template, "XXXXXX")) {
+		errno = EINVAL;
+		return NULL;
+	}
+
+	tmpdir = getenv("TMPDIR");
+	if (!tmpdir)
+		tmpdir = "/tmp";
+
+	strbuf_addf(&sb, "%s/%s", tmpdir, directory_template);
+	directorylen = sb.len;
+	if (!mkdtemp(sb.buf)) {
+		int orig_errno = errno;
+		strbuf_release(&sb);
+		errno = orig_errno;
+		return NULL;
+	}
+
+	strbuf_addf(&sb, "/%s", filename);
+	fd = open(sb.buf, O_CREAT | O_EXCL | O_RDWR, 0600);
+	if (fd < 0) {
+		int orig_errno = errno;
+		strbuf_setlen(&sb, directorylen);
+		rmdir(sb.buf);
+		strbuf_release(&sb);
+		errno = orig_errno;
+		return NULL;
+	}
+
+	tempfile = new_tempfile();
+	strbuf_swap(&tempfile->filename, &sb);
+	tempfile->directorylen = directorylen;
+	tempfile->fd = fd;
+	activate_tempfile(tempfile);
+	return tempfile;
+}
+
 struct tempfile *xmks_tempfile_m(const char *filename_template, int mode)
 {
 	struct tempfile *tempfile;
@@ -316,6 +378,7 @@ void delete_tempfile(struct tempfile **tempfile_p)

 	close_tempfile_gently(tempfile);
 	unlink_or_warn(tempfile->filename.buf);
+	remove_template_directory(tempfile, 0);
 	deactivate_tempfile(tempfile);
 	*tempfile_p = NULL;
 }
diff --git a/tempfile.h b/tempfile.h
index 4de3bc77d2..d7804a214a 100644
--- a/tempfile.h
+++ b/tempfile.h
@@ -82,6 +82,7 @@ struct tempfile {
 	FILE *volatile fp;
 	volatile pid_t owner;
 	struct strbuf filename;
+	size_t directorylen;
 };

 /*
@@ -198,6 +199,18 @@ static inline struct tempfile *xmks_tempfile(const char *filename_template)
 	return xmks_tempfile_m(filename_template, 0600);
 }

+/*
+ * Attempt to create a temporary directory in $TMPDIR and to create and
+ * open a file in that new directory. Derive the directory name from the
+ * template in the manner of mkdtemp(). Arrange for directory and file
+ * to be deleted if the program exits before they are deleted
+ * explicitly. On success return a tempfile whose "filename" member
+ * contains the full path of the file and its "fd" member is open for
+ * writing the file. On error return NULL and set errno appropriately.
+ */
+struct tempfile *mks_tempfile_dt(const char *directory_template,
+				 const char *filename);
+
 /*
  * Associate a stdio stream with the temporary file (which must still
  * be open). Return `NULL` (*without* deleting the file) on error. The
--
2.35.1