| Message ID | 20210817064435.97625-3-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | builtin/add: minor unrelated fixes | expand |
Hi Carlo, On Mon, 16 Aug 2021, Carlo Marcelo Arenas Belón wrote: > c59cb03a8b (git-add: introduce --edit (to edit the diff vs. the index), > 2009-04-08) add the option to add an edited patch directly to the index > interactively, but was silently ignored if any of the other interactive > options was also selected. > > report the user there is a conflict instead of silently ignoring -e > and while at it remove a variable assignment which was never used. > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > builtin/add.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/builtin/add.c b/builtin/add.c > index a15b5be220..be1920ab37 100644 > --- a/builtin/add.c > +++ b/builtin/add.c > @@ -308,7 +308,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix) > repo_init_revisions(the_repository, &rev, prefix); > rev.diffopt.context = 7; > > - argc = setup_revisions(argc, argv, &rev, NULL); > + setup_revisions(argc, argv, &rev, NULL); This looks fishy. I guess this was in reaction to some compiler warning that said that `argc` is not used after it was assigned? If that is the case, I would highly recommend against this hunk: the `setup_revisions()` function does alter the `argv` array, and `argc` is no longer necessarily correct afterwards. Sure, if there is no _current_ user of `argc` later in the code, you could remove that assignment Right Now. But future patches might need `argc` to be correct, and from experience I can tell you that those kinds of lurking bugs are no fun to figure out at all. So I'd say let's just drop this hunk. > rev.diffopt.output_format = DIFF_FORMAT_PATCH; > rev.diffopt.use_color = 0; > rev.diffopt.flags.ignore_dirty_submodules = 1; > @@ -486,6 +486,8 @@ int cmd_add(int argc, const char **argv, const char *prefix) > die(_("--dry-run is incompatible with --interactive/--patch")); > if (pathspec_from_file) > die(_("--pathspec-from-file is incompatible with --interactive/--patch")); > + if (edit_interactive) > + die(_("--edit-interactive is incompatible with --interactive/--patch")); This hunk, in contrast, makes a lot of sense to me. Both 1/2 and 2/2 (after dropping the first hunk of 2/2) are: `Acked-by` and/or `Reviewed-by` me, whichever you prefer. Thank you, Dscho > exit(interactive_add(argv + 1, prefix, patch_interactive)); > } > > -- > 2.33.0.476.gf000ecbed9 > >
diff --git a/builtin/add.c b/builtin/add.c index a15b5be220..be1920ab37 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -308,7 +308,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix) repo_init_revisions(the_repository, &rev, prefix); rev.diffopt.context = 7; - argc = setup_revisions(argc, argv, &rev, NULL); + setup_revisions(argc, argv, &rev, NULL); rev.diffopt.output_format = DIFF_FORMAT_PATCH; rev.diffopt.use_color = 0; rev.diffopt.flags.ignore_dirty_submodules = 1; @@ -486,6 +486,8 @@ int cmd_add(int argc, const char **argv, const char *prefix) die(_("--dry-run is incompatible with --interactive/--patch")); if (pathspec_from_file) die(_("--pathspec-from-file is incompatible with --interactive/--patch")); + if (edit_interactive) + die(_("--edit-interactive is incompatible with --interactive/--patch")); exit(interactive_add(argv + 1, prefix, patch_interactive)); }
c59cb03a8b (git-add: introduce --edit (to edit the diff vs. the index), 2009-04-08) add the option to add an edited patch directly to the index interactively, but was silently ignored if any of the other interactive options was also selected. report the user there is a conflict instead of silently ignoring -e and while at it remove a variable assignment which was never used. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- builtin/add.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)