Re: [PATCH 2/2] builtin/add: make clear edit and patch/interactive are incompatible
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Aug 17, 2021, 10:05 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2108171202250.55@tvgsbejvaqbjf.bet>
- In-Reply-To
- <20210817064435.97625-3-carenas@gmail.com>
Hi Carlo,
On Mon, 16 Aug 2021, Carlo Marcelo Arenas Belón wrote:
Show 23 quoted lines
> 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.
Show 9 quoted lines
> 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
Show 7 quoted lines
> exit(interactive_add(argv + 1, prefix, patch_interactive)); > } > > -- > 2.33.0.476.gf000ecbed9 > >