Re: [PATCH v4 4/4] add: modify already added files when --chmod is given

On 08/07, René Scharfe wrote:

Am 14.09.2016 um 23:07 schrieb Thomas Gummerer:

quoted

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.

Thanks for still replying :)

quoted

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

quoted

+{
+	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.

Thanks for catching this, both this and the above are worth changing
imo.  I see Ramsay already provided a patch for this in
https://public-inbox.org/git/aa004526-3e0d-66d4-287f-30abd29758fc@ramsayjones.plus.com/.
Thanks Ramsay!

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?

I'm not sure about this.  While I do agree that it could be quite
annoying in the last case, it could potentially be a bit confusing to
not get any warning in the first case.  I don't have a strong opinion
either way.

quoted

@@ -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.

I think this looks nicer indeed, thanks!  But it's probably not worth
a patch for just this unless we decide to change to not warn as you
mentioned above, and can fix this at the same time?

René