Re: [PATCH v4 4/4] add: modify already added files when --chmod is given
From: René Scharfe <hidden>
Date: 2017-08-07 21:40:58
Possibly related (same subject, not in this thread)
- 2016-09-14 · Re: [PATCH v4 4/4] add: modify already added files when --chmod is given · Junio C Hamano <hidden>
Am 14.09.2016 um 23:07 schrieb Thomas Gummerer:
When the chmod option was added to git add, it was hooked up to the diff machinery, meaning that it only works when the version in the index differs from the version on disk. As the option was supposed to mirror the chmod option in update-index, which always changes the mode in the index, regardless of the status of the file, make sure the option behaves the same way in git add. Signed-off-by: Thomas Gummerer <redacted>
Sorry for replying almost a year late, hopefully you're still interested.
quoted hunk ↗ jump to hunk
--- builtin/add.c | 47 ++++++++++++++++++++++++++++------------------- builtin/checkout.c | 2 +- builtin/commit.c | 2 +- cache.h | 10 +++++----- read-cache.c | 14 ++++++-------- t/t3700-add.sh | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 91 insertions(+), 34 deletions(-)diff --git a/builtin/add.c b/builtin/add.c index b1dddb4..595a0b2 100644 --- a/builtin/add.c +++ b/builtin/add.c@@ -26,10 +26,25 @@ static int patch_interactive, add_interactive, edit_interactive; static int take_worktree_changes; struct update_callback_data { - int flags, force_mode; + int flags; int add_errors; }; +static void chmod_pathspec(struct pathspec *pathspec, int force_mode)
"int force_mode" looks like a binary (or perhaps ternary) flag, but actually it is a character and can only have the values '-' or '+'. In builtin/update-index.c it's called "char flip" and we probably should define it like this here as well.
+{
+ int i;
+
+ for (i = 0; i < active_nr; i++) {
+ struct cache_entry *ce = active_cache[i];
+
+ if (pathspec && !ce_path_match(ce, pathspec, NULL))
+ continue;
+
+ if (chmod_cache_entry(ce, force_mode) < 0)
+ fprintf(stderr, "cannot chmod '%s'", ce->name);This error message is missing a newline. In builtin/update-index.c we also show the attempted change (-x or +x); perhaps we want to do that here as well. Currently chmod_cache_entry() can only fail if ce is not a regular file or it's other parameter is neither '-' nor '+'. We rule out the latter already in the argument parsing code. The former can happen if we add a symlink, either explicitly or because it's in a directory we're specified. I wonder if we even need to report anything, or under which conditions. If you have a file named dir/file and a symlink named dir/symlink then the interesting cases are: git add --chmod=.. dir/symlink git add --chmod=.. dir/file dir/symlink git add --chmod=.. dir Warning about each case may be the most cautious thing to do, but documenting that --chmod has no effect on symlinks and keeping silent might be less annoying, especially in the last case. What do you think?
quoted hunk ↗ jump to hunk
@@ -342,13 +354,8 @@ int cmd_add(int argc, const char **argv, const char *prefix) if (!show_only && ignore_missing) die(_("Option --ignore-missing can only be used together with --dry-run")); - 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 + if (chmod_arg && ((chmod_arg[0] != '-' && chmod_arg[0] != '+') || + chmod_arg[1] != 'x' || chmod_arg[2])) die(_("--chmod param '%s' must be either -x or +x"), chmod_arg);
That's the argument parsing code mentioned above. The strcmp-based checks look nicer to me btw. How about this? if (chmod_arg && strcmp(chmod_arg, "-x") && strcmp(chmod_arg, "+x")) But that's just nitpicking. René