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:
> > 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
>
> Sorry for replying almost a year late, hopefully you're still interested.
Thanks for still replying :)
> > ---
> > 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.
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-30abd2975...@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.
> > @@ -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é
Re: [PATCH v4 4/4] add: modify already added files when --chmod is given
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
Sorry for replying almost a year late, hopefully you're still interested.
> ---
> 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?
> @@ -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é
Re: [PATCH v4 4/4] add: modify already added files when --chmod is given
Thomas Gummerer writes:
> 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
> ---
This change essentially reverts most of what 4e55ed32 ("add: add
--chmod=+x / --chmod=-x options", 2016-05-31) did, except that it
keeps the command line option and adjusts its validation, and adds a
separate phase to "fix-up" the executable bits for all paths that
match the given pathspec, whether they were new or modified or
unchanged.
The patch makes sense to me. Thanks.
[PATCH v4 4/4] add: modify already added files when --chmod is given
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
---
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 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);
+ }
+}
+
static int fix_unmerged_status(struct diff_filepair *p,
struct update_callback_data *data)
{
@@ -65,8 +80,7 @@ static void update_callback(struct diff_queue_struct *q,
die(_("unexpected diff status %c"), p->status);
case DIFF_STATUS_MODIFIED:
case DIFF_STATUS_TYPE_CHANGED:
- if (add_file_to_index(&the_index, path,
- data->flags, data->force_mode)) {
+ if (add_file_to_index(&the_index, path, data->flags)) {
if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))
die(_("updating files failed"));
data->add_errors++;
@@ -84,15 +98,14 @@ static void update_callback(struct diff_queue_struct *q,
}
}
-int add_files_to_cache(const char *prefix, const struct pathspec *pathspec,
- int flags, int force_mode)
+int add_files_to_cache(const char *prefix,
+ const struct pathspec *pathspec, int flags)
{
struct update_callback_data data;
struct rev_info rev;
memset(&data, 0, sizeof(data));
data.flags = flags;
- data.force_mode = force_mode;
init_revisions(&rev, prefix);
setup_revisions(0, NULL, &rev, NULL);
@@ -281,7 +294,7 @@ static int add_config(const char *var, const char *value,
void *cb)
return git_default_config(var, value, cb);
}
-static int add_files(struct dir_struct *dir, int flags, int force_mode)
+static int add_files(struct dir_struct *dir, int flags)
{
int i, exit_status = 0;
@@ -294,8 +307,7 @@ static int add_files(struct dir_struct *dir, int flags, int
force_mode)
}
for (i = 0; i < dir->nr; i++)
- if (add_file_to_index(&the_index, dir->entries[i]->name,
- flags, force_mode)) {
+ if (add_file_to_index(&the_index, dir->entries[i]->name,
flags)) {
if (!ignore_add_errors)
die(_("adding files failed"));
exit_status = 1;
@@ -308,7 +320,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
int exit_status = 0;
struct pathspec pathspec;
struct dir_struct dir;
- int flags, force_mode;
+ int flags;
int add_new_files;
int require_pathspec;
char *seen = NULL;
@@ -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);
add_new_files = !take_worktree_changes && !refresh_only;
@@ -441,11 +448,13 @@ int cmd_add(int argc, const char **argv, const char
*prefix)
plug_bulk_checkin();
- exit_status |= add_files_to_cache(prefix, &pathspec, flags, force_mode);
+ exit_status |= ad
