Re: [PATCH 5/5] commit: support the --pathspec-from-file option

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Alexandr

Sorry it has taken me so long to reply

On 06/11/2019 15:56, Alexandr Miloslavskiy wrote:
> I think I have implemented most suggestions in PatchV2. Thanks!
> 
>> It might be worth tailoring the message to the command rather than 
>> having exactly the same message for commit and reset
> 
> I decided to move the general comment to base commit where options are 
> introduced and not repeat it where options are supported.
> 
>> I wonder if there is a way of calling parse_pathspec_file() from 
>> parse_and_validate_options() instead. Otherwise we end up validating 
>> options here instead which is a bit messy.
> 
> The code looks a bit too entangled to support that without making it 
> worse. The biggest concern I have is that parse_and_validate_options() 
> will populate `pathspec` and some other function will need to remember 
> to clean it up. I like it better when `pathspec` is handled in one place.

I don't think it's so bad if the pathspec is cleaned up just after it is used, 
the diff below applies on top of your patch - see what you think. The diff 
also adds dies if --all is given with --pathspec-from-file which (together 
with a test) would be worth adding to you series I think.

> 
> I agree that things are not perfect, but this seems to be a consequence 
> of other existing problems. For example, I would have expected a 
> structure instead of a handful of global variables. That would have 
> solved many problems. However, I didn't have the bravery to dive into 
> this refactoring.

Yes it is a pain that the builtin functions tend to use a lot of global 
variables rather than a structure.

Best Wishes

Phillip

--- >8 ---
diff --git a/builtin/commit.c b/builtin/commit.c
index ed40729355..bb9515c44b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -330,37 +330,18 @@ static void refresh_cache_or_die(int refresh_flags)
 }
 
 static const char *prepare_index(int argc, const char **argv, const char *prefix,
+				 struct pathspec *pathspec,
 				 const struct commit *current_head, int is_status)
 {
 	struct string_list partial = STRING_LIST_INIT_DUP;
-	struct pathspec pathspec;
+
 	int refresh_flags = REFRESH_QUIET;
 	const char *ret;
 
 	if (is_status)
 		refresh_flags |= REFRESH_UNMERGED;
-	parse_pathspec(&pathspec, 0,
-		       PATHSPEC_PREFER_FULL,
-		       prefix, argv);
 
-	if (pathspec_from_file) {
-		if (interactive)
-			die(_("--pathspec-from-file is incompatible with --interactive/--patch"));
-
-		if (pathspec.nr)
-			die(_("--pathspec-from-file is incompatible with pathspec arguments"));
-
-		parse_pathspec_file(&pathspec, 0,
-				    PATHSPEC_PREFER_FULL,
-				    prefix, pathspec_from_file, pathspec_file_nul);
-	} else if (pathspec_file_nul) {
-		die(_("--pathspec-file-nul requires --pathspec-from-file"));
-	}
-
-	if (!pathspec.nr && (also || (only && !amend && !allow_empty)))
-		die(_("No paths with --include/--only does not make sense."));
-
-	if (read_cache_preload(&pathspec) < 0)
+	if (read_cache_preload(pathspec) < 0)
 		die(_("index file corrupt"));
 
 	if (interactive) {
@@ -411,9 +392,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 	 * (A) if all goes well, commit the real index;
 	 * (B) on failure, rollback the real index.
 	 */
-	if (all || (also && pathspec.nr)) {
+	if (all || (also && pathspec->nr)) {
 		hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
-		add_files_to_cache(also ? prefix : NULL, &pathspec, 0);
+		add_files_to_cache(also ? prefix : NULL, pathspec, 0);
 		refresh_cache_or_die(refresh_flags);
 		update_main_cache_tree(WRITE_TREE_SILENT);
 		if (write_locked_index(&the_index, &index_lock, 0))
@@ -432,7 +413,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 	 * and create commit from the_index.
 	 * We still need to refresh the index here.
 	 */
-	if (!only && !pathspec.nr) {
+	if (!only && !pathspec->nr) {
 		hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
 		refresh_cache_or_die(refresh_flags);
 		if (active_cache_changed
@@ -474,7 +455,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 			die(_("cannot do a partial commit during a cherry-pick."));
 	}
 
-	if (list_paths(&partial, !current_head ? NULL : "HEAD", &pathspec))
+	if (list_paths(&partial, !current_head ? NULL : "HEAD", pathspec))
 		exit(1);
 
 	discard_cache();
@@ -505,7 +486,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 	read_cache_from(ret);
 out:
 	string_list_clear(&partial, 0);
-	clear_pathspec(&pathspec);
+	clear_pathspec(pathspec);
 	return ret;
 }
 
@@ -1148,6 +1129,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 				      const struct option *options,
 				      const char * const usage[],
 				      const char *prefix,
+				      struct pathspec *pathspec,
 				      struct commit *current_head,
 				      struct wt_status *s)
 {
@@ -1223,19 +1205,42 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		die(_("paths '%s ...' with -a does not make sense"),
 		    argv[0]);
 
+	if (pathspec_from_file) {
+		if (interactive)
+			die(_("--pathspec-from-file is incompatible with --interactive/--patch"));
+
+		if (argc)
+			die(_("--pathspec-from-file is incompatible with pathspec arguments"));
+
+		if (all)
+			die(_("--pathspec-from-file is incompatible with --all"));
+
+		parse_pathspec_file(pathspec, 0,
+				    PATHSPEC_PREFER_FULL,
+				    prefix, pathspec_from_file, pathspec_file_nul);
+	} else if (pathspec_file_nul) {
+		die(_("--pathspec-file-nul requires --pathspec-from-file"));
+	} else {
+		parse_pathspec(pathspec, 0, PATHSPEC_PREFER_FULL, prefix, argv);
+	}
+
+	if (!pathspec->nr && (also || (only && !amend && !allow_empty)))
+		die(_("No paths with --include/--only does not make sense."));
+
 	if (status_format != STATUS_FORMAT_NONE)
 		dry_run = 1;
 
 	return argc;
 }
 
 static int dry_run_commit(int argc, const char **argv, const char *prefix,
+			  struct pathspec *pathspec,
 			  const struct commit *current_head, struct wt_status *s)
 {
 	int committable;
 	const char *index_file;
 
-	index_file = prepare_index(argc, argv, prefix, current_head, 1);
+	index_file = prepare_index(argc, argv, prefix, pathspec, current_head, 1);
 	committable = run_status(stdout, index_file, prefix, 0, s);
 	rollback_index_files();
 
@@ -1571,6 +1576,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	struct commit *current_head = NULL;
 	struct commit_extra_header *extra = NULL;
 	struct strbuf err = STRBUF_INIT;
+	struct pathspec pathspec;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_commit_usage, builtin_commit_options);
@@ -1590,13 +1596,15 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	verbose = -1; /* unspecified */
 	argc = parse_and_validate_options(argc, argv, builtin_commit_options,
 					  builtin_commit_usage,
-					  prefix, current_head, &s);
+					  prefix, &pathspec, current_head, &s);
 	if (verbose == -1)
 		verbose = (config_commit_verbose < 0) ? 0 : config_commit_verbose;
 
 	if (dry_run)
-		return dry_run_commit(argc, argv, prefix, current_head, &s);
-	index_file = prepare_index(argc, argv, prefix, current_head, 0);
+		return dry_run_commit(argc, argv, prefix, &pathspec,
+				     current_head, &s);
+	index_file = prepare_index(argc, argv, prefix, &pathspec, current_head,
+				   0);
 
 	/* Set up everything for writing the commit object.  This includes
 	   running hooks, writing the trees, and interacting with the user.  */