[PATCH 0/2] builtin/add: minor unrelated fixes

[PATCH 0/2] builtin/add: minor unrelated fixes

[Carlo Marcelo Arenas Belón]

While not related, they touch nearby code and had the same author.

Carlo Marcelo Arenas Belón (2):
  builtin/add: remove obsoleted support for legacy stash -p
  builtin/add: make clear edit and patch/interactive are incompatible

 builtin/add.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

-- 
2.33.0.476.gf000ecbed9

[PATCH 1/2] builtin/add: remove obsoleted support for legacy stash -p

[Carlo Marcelo Arenas Belón]

90a6bb98d1 (legacy stash -p: respect the add.interactive.usebuiltin
setting, 2019-12-21) adds a hidden option and its supporting code
to support the legacy stash script, but that was left behind when
it was retired.

mostly revert commit.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 builtin/add.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index b773b5a499..a15b5be220 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -31,7 +31,6 @@ static int take_worktree_changes;
 static int add_renormalize;
 static int pathspec_file_nul;
 static const char *pathspec_from_file;
-static int legacy_stash_p; /* support for the scripted `git stash` */
 
 struct update_callback_data {
 	int flags;
@@ -382,8 +381,6 @@ static struct option builtin_add_options[] = {
 		   N_("override the executable bit of the listed files")),
 	OPT_HIDDEN_BOOL(0, "warn-embedded-repo", &warn_on_embedded_repo,
 			N_("warn when adding an embedded repository")),
-	OPT_HIDDEN_BOOL(0, "legacy-stash-p", &legacy_stash_p,
-			N_("backend for `git stash -p`")),
 	OPT_PATHSPEC_FROM_FILE(&pathspec_from_file),
 	OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul),
 	OPT_END(),
@@ -483,6 +480,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			  builtin_add_usage, PARSE_OPT_KEEP_ARGV0);
 	if (patch_interactive)
 		add_interactive = 1;
+
 	if (add_interactive) {
 		if (show_only)
 			die(_("--dry-run is incompatible with --interactive/--patch"));
@@ -490,17 +488,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			die(_("--pathspec-from-file is incompatible with --interactive/--patch"));
 		exit(interactive_add(argv + 1, prefix, patch_interactive));
 	}
-	if (legacy_stash_p) {
-		struct pathspec pathspec;
-
-		parse_pathspec(&pathspec, 0,
-			PATHSPEC_PREFER_FULL |
-			PATHSPEC_SYMLINK_LEADING_PATH |
-			PATHSPEC_PREFIX_ORIGIN,
-			prefix, argv);
-
-		return run_add_interactive(NULL, "--patch=stash", &pathspec);
-	}
 
 	if (edit_interactive) {
 		if (pathspec_from_file)
-- 
2.33.0.476.gf000ecbed9

[PATCH 2/2] builtin/add: make clear edit and patch/interactive are incompatible

[Carlo Marcelo Arenas Belón]

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);
 	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));
 	}
 
-- 
2.33.0.476.gf000ecbed9

Re: [PATCH 2/2] builtin/add: make clear edit and patch/interactive are incompatible

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2500 bytes --]

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

Re: [PATCH 1/2] builtin/add: remove obsoleted support for legacy stash -p

[Taylor Blau]

On Mon, Aug 16, 2021 at 11:44:34PM -0700, Carlo Marcelo Arenas Belón wrote:
> 90a6bb98d1 (legacy stash -p: respect the add.interactive.usebuiltin
> setting, 2019-12-21) adds a hidden option and its supporting code
> to support the legacy stash script, but that was left behind when
> it was retired.
>
> mostly revert commit.

(Sorry for a much-delayed response, I'm trying to do a little bit of
inbox-cleaning ;)).

If you're re-rolling based on Dscho's suggestions later on in the
series, I'd suggest two changes to make the patch message clearer:

  - Clarify the antecedent of "it" in "it was retired". I find that
    "...but that [option] was forgotten about even when [the legacy
    stash implementation] was retired".

  - Link to the commit where we dropped support for that implementation
    (which was in 8a2cd3f512 (stash: remove the stash.useBuiltin
    setting, 2020-03-03)) to make it clear that that happened after
    90a6bb98d1.

  - Finally "mostly revert commit." should add more detail without being
    verbose. I'd write:

        Since 8a2cd3f512 removed the legacy implementation, the changes
        from 90a6bb98d1 are no longer necessary, so revert them.

> @@ -483,6 +480,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  			  builtin_add_usage, PARSE_OPT_KEEP_ARGV0);
>  	if (patch_interactive)
>  		add_interactive = 1;
> +

Stray whitespace change, but the rest of the patch looks good to me.

Thanks,
Taylor