Re: [PATCH v2 2/4] update-index: use the same structure for chmod as add
From: Junio C Hamano <hidden>
Date: 2016-09-11 22:28:35
Thomas Gummerer [off-list ref] writes:
quoted hunk
@@ -955,10 +941,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) PARSE_OPT_NOARG | /* disallow --cacheinfo=<mode> form */ PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP, (parse_opt_cb *) cacheinfo_callback}, - {OPTION_CALLBACK, 0, "chmod", &set_executable_bit, N_("(+/-)x"), - N_("override the executable bit of the listed files"), - PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP, - chmod_callback}, + OPT_STRING( 0, "chmod", &chmod_arg, N_("(+/-)x"), + N_("override the executable bit of the listed files")), {OPTION_SET_INT, 0, "assume-unchanged", &mark_valid_only, NULL, N_("mark files as \"not changing\""), PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},@@ -1018,6 +1002,15 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) if (argc == 2 && !strcmp(argv[1], "-h")) usage_with_options(update_index_usage, options); + if (!chmod_arg) + force_mode = 0; + else if (!strcmp(chmod_arg, "-x")) + force_mode = 0666; + else if (!strcmp(chmod_arg, "+x")) + force_mode = 0777; + else + die(_("option 'chmod' expects \"+x\" or \"-x\"")); +
I am afraid that this changes the behaviour drastically. "git update-index" is an oddball command that takes options and then processes them immediately, exactly because it was designed to take git update-index --chmod=-x A --chmod=+x B --add C and say things like "A and B are not in the index and you are attempting to add them before giving me --add option". git update-index --add --chmod=-x A --chmod=+x B C is expected to add A as non-executable, and B and C as executable. Many exotic parse-options callback mechanisms used in this command were invented exactly to support its quirky way of not doing "get a list of options and use the last one". And this patch breaks it for only one option without changing the others. If we were willing to take such a big backward compatiblity hit in the upcoming release (which I personally won't be affected, but old scripts by others need to be audited and adjusted, which I won't volunteer to do myself), we should make such a change consistently, e.g. "git update-index A --add --remove B" should no longer error out when it sees A and it is not yet in the index because "--add" hasn't been given yet, or A is in the index but is missing from the working tree because "--remove" hasn't been given yet. Then it may be more justifiable if "update-index --chmod=-x A --chmod=+x B" added A as an executable. With the current form of this patch, it is not. Can we do this "fix" without this change?