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