Re: [PATCH 2/2] builtin/add: make clear edit and patch/interactive are incompatible
flat view
From: Johannes Schindelin <hidden>
Date: 2021-08-17 10:07:22
Hi Carlo, On Mon, 16 Aug 2021, Carlo Marcelo Arenas Belón wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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.
quoted hunk ↗ jump to 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