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. */
