Re: [ANNOUNCE] Git v2.29.0-rc1

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

Am 12.10.20 um 21:19 schrieb René Scharfe:
> Am 12.10.20 um 18:33 schrieb Junio C Hamano:
>> If this "feature" were experimental and if the experiment turns out
>> to be a failure, would we have a viable alternative definition?
>>
>> Perhaps "--add-file names an untracked file in the working tree and
>> the single '--prefix' that is used for entries that come from the
>> tree object is applied"?  Or perhaps remove --add-file entirely as a
>> failed experiment?
>
> Removing --add-file entirely is certainly possible, but it's used in the
> Makefile now and I can't imagine what would make its disposal necessary.
>
> Turning it into a standard OPT_STRING_LIST option for full untracked
> paths and using the last --prefix value for all archive entries would be
> a more straightforward UI and might be versatile enough.

Here's how that would have looked, but it seems I'm too late.

The Makefile rules get a bit more fragile because the version files are
special and the simpler --add-file forces us to create them at their
actual place if not present, which can lead to problems (stuck version)
if we don't delete them afterwards.  Avoiding that was the reason for
the more complicated version that ended up in v2.29.  That fragility
could be defused by teaching GIT-VERSION-GEN about version_is_generated.

Anyway, at this point I'm OK with keeping the released version to avoid
additional confusion.


 Documentation/git-archive.txt |  4 +--
 Makefile                      | 52 +++++++++++++++------------
 archive.c                     | 83 ++++++++++++++++---------------------------
 t/t5000-tar-tree.sh           | 11 ------
 t/t5003-archive-zip.sh        | 10 ------
 5 files changed, 61 insertions(+), 99 deletions(-)

diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt
index 9f8172828d..b221dd5e3d 100644
--- a/Documentation/git-archive.txt
+++ b/Documentation/git-archive.txt
@@ -57,9 +57,7 @@ OPTIONS

 --add-file=<file>::
 	Add a non-tracked file to the archive.  Can be repeated to add
-	multiple files.  The path of the file in the archive is built
-	by concatenating the value for `--prefix` (if any) and the
-	basename of <file>.
+	multiple files.

 --worktree-attributes::
 	Look for attributes in .gitattributes files in the working tree
diff --git a/Makefile b/Makefile
index 1fb0ec1705..33fd806bc6 100644
--- a/Makefile
+++ b/Makefile
@@ -3050,34 +3050,41 @@ quick-install-html:

 ### Maintainer's dist rules

+version:
+	touch version_is_generated
+	echo $(GIT_VERSION) > version
+
+git-gui/version:
+	touch git-gui/version_is_generated
+	$(MAKE) -C git-gui TARDIR=. dist-version
+
+define version_clean_script =
+if test -f version_is_generated; then \
+	$(RM) version version_is_generated; \
+fi
+if test -f git-gui/version_is_generated; then \
+	$(RM) git-gui/version git-gui/version_is_generated; \
+fi
+endef
+
 # Allow tweaking to hide local environment effects, like perm bits.
 # With GNU tar, "--mode=u+rwX,og+rX,og-w" would be a good idea, for example.
 TAR_DIST_EXTRA_OPTS =
 GIT_TARNAME = git-$(GIT_VERSION)
-GIT_ARCHIVE_EXTRA_FILES = \
-	--prefix=$(GIT_TARNAME)/ \
-	--add-file=configure \
-	--add-file=$(GIT_TARNAME)/version \
-	--prefix=$(GIT_TARNAME)/git-gui/ \
-	--add-file=$(GIT_TARNAME)/git-gui/version
+GIT_ARCHIVE_EXTRA_FILES = configure version git-gui/version
 ifdef DC_SHA1_SUBMODULE
-GIT_ARCHIVE_EXTRA_FILES += \
-	--prefix=$(GIT_TARNAME)/sha1collisiondetection/ \
-	--add-file=sha1collisiondetection/LICENSE.txt \
-	--prefix=$(GIT_TARNAME)/sha1collisiondetection/lib/ \
-	--add-file=sha1collisiondetection/lib/sha1.c \
-	--add-file=sha1collisiondetection/lib/sha1.h \
-	--add-file=sha1collisiondetection/lib/ubc_check.c \
-	--add-file=sha1collisiondetection/lib/ubc_check.h
-endif
-dist: git-archive$(X) configure
-	@mkdir -p $(GIT_TARNAME)
-	@echo $(GIT_VERSION) > $(GIT_TARNAME)/version
-	@$(MAKE) -C git-gui TARDIR=../$(GIT_TARNAME)/git-gui dist-version
+GIT_ARCHIVE_EXTRA_FILES += sha1collisiondetection/LICENSE.txt \
+			   sha1collisiondetection/lib/sha1.c \
+			   sha1collisiondetection/lib/sha1.h \
+			   sha1collisiondetection/lib/ubc_check.c \
+			   sha1collisiondetection/lib/ubc_check.h
+endif
+dist: git-archive$(X) configure version git-gui/version
 	./git-archive --format=tar \
-		$(GIT_ARCHIVE_EXTRA_FILES) \
-		--prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar
-	@$(RM) -r $(GIT_TARNAME)
+		$(subst $(space),$(space)--add-file=,$(space)$(GIT_ARCHIVE_EXTRA_FILES)) \
+		--prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar || \
+	$(RM) $(GIT_TARNAME).tar
+	$(version_clean_script)
 	gzip -f -9 $(GIT_TARNAME).tar

 rpm::
@@ -3164,6 +3171,7 @@ endif
 	$(RM) GIT-VERSION-FILE GIT-CFLAGS GIT-LDFLAGS GIT-BUILD-OPTIONS
 	$(RM) GIT-USER-AGENT GIT-PREFIX
 	$(RM) GIT-SCRIPT-DEFINES GIT-PERL-DEFINES GIT-PERL-HEADER GIT-PYTHON-VARS
+	$(version_clean_script)
 ifdef MSVC
 	$(RM) $(patsubst %.o,%.o.pdb,$(OBJECTS))
 	$(RM) $(patsubst %.exe,%.pdb,$(OTHER_PROGRAMS))
diff --git a/archive.c b/archive.c
index 3c1541af9e..bb56b27581 100644
--- a/archive.c
+++ b/archive.c
@@ -267,8 +267,8 @@ static int queue_or_write_archive_entry(const struct object_id *oid,
 }

 struct extra_file_info {
-	char *base;
-	struct stat stat;
+	unsigned int mode;
+	size_t size;
 };

 int write_archive_entries(struct archiver_args *args,
@@ -278,7 +278,6 @@ int write_archive_entries(struct archiver_args *args,
 	struct unpack_trees_options opts;
 	struct tree_desc t;
 	int err;
-	struct strbuf path_in_archive = STRBUF_INIT;
 	struct strbuf content = STRBUF_INIT;
 	struct object_id fake_oid = null_oid;
 	int i;
@@ -331,27 +330,24 @@ int write_archive_entries(struct archiver_args *args,
 	for (i = 0; i < args->extra_files.nr; i++) {
 		struct string_list_item *item = args->extra_files.items + i;
 		char *path = item->string;
+		char *path_on_disk = prefix_filename(args->prefix, path);
+		char *path_in_archive = prefix_filename(args->base, path);
 		struct extra_file_info *info = item->util;

 		put_be64(fake_oid.hash, i + 1);

-		strbuf_reset(&path_in_archive);
-		if (info->base)
-			strbuf_addstr(&path_in_archive, info->base);
-		strbuf_addstr(&path_in_archive, basename(path));
-
 		strbuf_reset(&content);
-		if (strbuf_read_file(&content, path, info->stat.st_size) < 0)
+		if (strbuf_read_file(&content, path_on_disk, info->size) < 0)
 			err = error_errno(_("could not read '%s'"), path);
 		else
-			err = write_entry(args, &fake_oid, path_in_archive.buf,
-					  path_in_archive.len,
-					  info->stat.st_mode,
+			err = write_entry(args, &fake_oid, path_in_archive,
+					  strlen(path_in_archive), info->mode,
 					  content.buf, content.len);
+		free(path_on_disk);
+		free(path_in_archive);
 		if (err)
 			break;
 	}
-	strbuf_release(&path_in_archive);
 	strbuf_release(&content);

 	return err;
@@ -493,42 +489,6 @@ static void parse_treeish_arg(const char **argv,
 	ar_args->time = archive_time;
 }

-static void extra_file_info_clear(void *util, const char *str)
-{
-	struct extra_file_info *info = util;
-	free(info->base);
-	free(info);
-}
-
-static int add_file_cb(const struct option *opt, const char *arg, int unset)
-{
-	struct archiver_args *args = opt->value;
-	const char **basep = (const char **)opt->defval;
-	const char *base = *basep;
-	char *path;
-	struct string_list_item *item;
-	struct extra_file_info *info;
-
-	if (unset) {
-		string_list_clear_func(&args->extra_files,
-				       extra_file_info_clear);
-		return 0;
-	}
-
-	if (!arg)
-		return -1;
-
-	path = prefix_filename(args->prefix, arg);
-	item = string_list_append_nodup(&args->extra_files, path);
-	item->util = info = xmalloc(sizeof(*info));
-	info->base = xstrdup_or_null(base);
-	if (stat(path, &info->stat))
-		die(_("File not found: %s"), path);
-	if (!S_ISREG(info->stat.st_mode))
-		die(_("Not a regular file: %s"), path);
-	return 0;
-}
-
 #define OPT__COMPR(s, v, h, p) \
 	OPT_SET_INT_F(s, NULL, v, h, p, PARSE_OPT_NONEG)
 #define OPT__COMPR_HIDDEN(s, v, p) \
@@ -548,14 +508,14 @@ static int parse_archive_args(int argc, const char **argv,
 	int i;
 	int list = 0;
 	int worktree_attributes = 0;
+	struct string_list_item *item;
 	struct option opts[] = {
 		OPT_GROUP(""),
 		OPT_STRING(0, "format", &format, N_("fmt"), N_("archive format")),
 		OPT_STRING(0, "prefix", &base, N_("prefix"),
 			N_("prepend prefix to each pathname in the archive")),
-		{ OPTION_CALLBACK, 0, "add-file", args, N_("file"),
-		  N_("add untracked file to archive"), 0, add_file_cb,
-		  (intptr_t)&base },
+		OPT_STRING_LIST(0, "add-file", &args->extra_files, N_("file"),
+				N_("add untracked file to archive")),
 		OPT_STRING('o', "output", &output, N_("file"),
 			N_("write the archive to this file")),
 		OPT_BOOL(0, "worktree-attributes", &worktree_attributes,
@@ -593,6 +553,23 @@ static int parse_archive_args(int argc, const char **argv,
 	if (is_remote && args->extra_files.nr)
 		die(_("Options --add-file and --remote cannot be used together"));

+	for_each_string_list_item(item, &args->extra_files) {
+		char *path;
+		struct stat st;
+		struct extra_file_info *info;
+
+		path = prefix_filename(args->prefix, item->string);
+		if (lstat(path, &st))
+			die(_("File not found: %s"), item->string);
+		if (!S_ISREG(st.st_mode))
+			die(_("Not a regular file: %s"), item->string);
+		free(path);
+
+		item->util = info = xmalloc(sizeof(*info));
+		info->mode = canon_mode(st.st_mode);
+		info->size = st.st_size;
+	}
+
 	if (!base)
 		base = "";

@@ -661,7 +638,7 @@ int write_archive(int argc, const char **argv, const char *prefix,

 	rc = ar->write_archive(ar, &args);

-	string_list_clear_func(&args.extra_files, extra_file_info_clear);
+	string_list_clear(&args.extra_files, 1);

 	return rc;
 }
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 3ebb0d3b65..a090712ddb 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -182,17 +182,6 @@ test_expect_success 'git archive --add-file' '
 check_tar with_untracked
 check_added with_untracked untracked untracked

-test_expect_success 'git archive --add-file twice' '
-	echo untracked >untracked &&
-	git archive --prefix=one/ --add-file=untracked \
-		--prefix=two/ --add-file=untracked \
-		--prefix= HEAD >with_untracked2.tar
-'
-
-check_tar with_untracked2
-check_added with_untracked2 untracked one/untracked
-check_added with_untracked2 untracked two/untracked
-
 test_expect_success 'git archive on large files' '
     test_config core.bigfilethreshold 1 &&
     git archive HEAD >b3.tar &&
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index 1e6d18b140..a9a44f2760 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -206,14 +206,4 @@ test_expect_success 'git archive --format=zip --add-file' '
 check_zip with_untracked
 check_added with_untracked untracked untracked

-test_expect_success 'git archive --format=zip --add-file twice' '
-	echo untracked >untracked &&
-	git archive --format=zip --prefix=one/ --add-file=untracked \
-		--prefix=two/ --add-file=untracked \
-		--prefix= HEAD >with_untracked2.zip
-'
-check_zip with_untracked2
-check_added with_untracked2 untracked one/untracked
-check_added with_untracked2 untracked two/untracked
-
 test_done