threads / patch / 56303
Subject: [PATCH 0/2] builtin/add: minor unrelated fixes
Carlo Marcelo Arenas BelónWhile 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
Carlo Marcelo Arenas Belón[PATCH 1/2] builtin/add: remove obsoleted support for legacy stash -p
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
Taylor BlauRe: [PATCH 1/2] builtin/add: remove obsoleted support for legacy stash -p
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
Carlo Marcelo Arenas Belón[PATCH 2/2] builtin/add: make clear edit and patch/interactive are incompatible
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
Johannes SchindelinRe: [PATCH 2/2] builtin/add: make clear edit and patch/interactive are incompatible
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 > >