{"thread":{"id":"56303","subject":"[PATCH 0/2] builtin/add: minor unrelated fixes","startedAt":"2021-08-17T06:44:50Z","lastAt":"2021-08-31T00:33:48Z","messageCount":5,"participants":["Carlo Marcelo Arenas Belón","Johannes Schindelin","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"432901","messageId":"20210817064435.97625-1-carenas@gmail.com","threadId":"56303","inReplyTo":null,"subject":"[PATCH 0/2] builtin/add: minor unrelated fixes","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-08-17T06:44:33Z","receivedAt":"2021-08-17T06:44:50Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"While not related, they touch nearby code and had the same author.\n\nCarlo Marcelo Arenas Belón (2):\n  builtin/add: remove obsoleted support for legacy stash -p\n  builtin/add: make clear edit and patch/interactive are incompatible\n\n builtin/add.c | 19 ++++---------------\n 1 file changed, 4 insertions(+), 15 deletions(-)\n\n-- \n2.33.0.476.gf000ecbed9\n\n"},{"id":"432902","messageId":"20210817064435.97625-2-carenas@gmail.com","threadId":"56303","inReplyTo":"20210817064435.97625-1-carenas@gmail.com","subject":"[PATCH 1/2] builtin/add: remove obsoleted support for legacy stash -p","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-08-17T06:44:34Z","receivedAt":"2021-08-17T06:44:51Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"90a6bb98d1 (legacy stash -p: respect the add.interactive.usebuiltin\nsetting, 2019-12-21) adds a hidden option and its supporting code\nto support the legacy stash script, but that was left behind when\nit was retired.\n\nmostly revert commit.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n builtin/add.c | 15 +--------------\n 1 file changed, 1 insertion(+), 14 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex b773b5a499..a15b5be220 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -31,7 +31,6 @@ static int take_worktree_changes;\n static int add_renormalize;\n static int pathspec_file_nul;\n static const char *pathspec_from_file;\n-static int legacy_stash_p; /* support for the scripted `git stash` */\n \n struct update_callback_data {\n \tint flags;\n@@ -382,8 +381,6 @@ static struct option builtin_add_options[] = {\n \t\t   N_(\"override the executable bit of the listed files\")),\n \tOPT_HIDDEN_BOOL(0, \"warn-embedded-repo\", &warn_on_embedded_repo,\n \t\t\tN_(\"warn when adding an embedded repository\")),\n-\tOPT_HIDDEN_BOOL(0, \"legacy-stash-p\", &legacy_stash_p,\n-\t\t\tN_(\"backend for `git stash -p`\")),\n \tOPT_PATHSPEC_FROM_FILE(&pathspec_from_file),\n \tOPT_PATHSPEC_FILE_NUL(&pathspec_file_nul),\n \tOPT_END(),\n@@ -483,6 +480,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\t  builtin_add_usage, PARSE_OPT_KEEP_ARGV0);\n \tif (patch_interactive)\n \t\tadd_interactive = 1;\n+\n \tif (add_interactive) {\n \t\tif (show_only)\n \t\t\tdie(_(\"--dry-run is incompatible with --interactive/--patch\"));\n@@ -490,17 +488,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"--pathspec-from-file is incompatible with --interactive/--patch\"));\n \t\texit(interactive_add(argv + 1, prefix, patch_interactive));\n \t}\n-\tif (legacy_stash_p) {\n-\t\tstruct pathspec pathspec;\n-\n-\t\tparse_pathspec(&pathspec, 0,\n-\t\t\tPATHSPEC_PREFER_FULL |\n-\t\t\tPATHSPEC_SYMLINK_LEADING_PATH |\n-\t\t\tPATHSPEC_PREFIX_ORIGIN,\n-\t\t\tprefix, argv);\n-\n-\t\treturn run_add_interactive(NULL, \"--patch=stash\", &pathspec);\n-\t}\n \n \tif (edit_interactive) {\n \t\tif (pathspec_from_file)\n-- \n2.33.0.476.gf000ecbed9\n\n"},{"id":"432903","messageId":"20210817064435.97625-3-carenas@gmail.com","threadId":"56303","inReplyTo":"20210817064435.97625-1-carenas@gmail.com","subject":"[PATCH 2/2] builtin/add: make clear edit and patch/interactive are incompatible","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-08-17T06:44:35Z","receivedAt":"2021-08-17T06:44:53Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"c59cb03a8b (git-add: introduce --edit (to edit the diff vs. the index),\n2009-04-08) add the option to add an edited patch directly to the index\ninteractively, but was silently ignored if any of the other interactive\noptions was also selected.\n\nreport the user there is a conflict instead of silently ignoring -e\nand while at it remove a variable assignment which was never used.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n builtin/add.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex a15b5be220..be1920ab37 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -308,7 +308,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n \trepo_init_revisions(the_repository, &rev, prefix);\n \trev.diffopt.context = 7;\n \n-\targc = setup_revisions(argc, argv, &rev, NULL);\n+\tsetup_revisions(argc, argv, &rev, NULL);\n \trev.diffopt.output_format = DIFF_FORMAT_PATCH;\n \trev.diffopt.use_color = 0;\n \trev.diffopt.flags.ignore_dirty_submodules = 1;\n@@ -486,6 +486,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"--dry-run is incompatible with --interactive/--patch\"));\n \t\tif (pathspec_from_file)\n \t\t\tdie(_(\"--pathspec-from-file is incompatible with --interactive/--patch\"));\n+\t\tif (edit_interactive)\n+\t\t\tdie(_(\"--edit-interactive is incompatible with --interactive/--patch\"));\n \t\texit(interactive_add(argv + 1, prefix, patch_interactive));\n \t}\n \n-- \n2.33.0.476.gf000ecbed9\n\n"},{"id":"432933","messageId":"nycvar.QRO.7.76.6.2108171202250.55@tvgsbejvaqbjf.bet","threadId":"56303","inReplyTo":"20210817064435.97625-3-carenas@gmail.com","subject":"Re: [PATCH 2/2] builtin/add: make clear edit and patch/interactive are incompatible","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-08-17T10:05:44Z","receivedAt":"2021-08-17T10:07:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Mon, 16 Aug 2021, Carlo Marcelo Arenas Belón wrote:\n\n> c59cb03a8b (git-add: introduce --edit (to edit the diff vs. the index),\n> 2009-04-08) add the option to add an edited patch directly to the index\n> interactively, but was silently ignored if any of the other interactive\n> options was also selected.\n>\n> report the user there is a conflict instead of silently ignoring -e\n> and while at it remove a variable assignment which was never used.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  builtin/add.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/add.c b/builtin/add.c\n> index a15b5be220..be1920ab37 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -308,7 +308,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n>  \trepo_init_revisions(the_repository, &rev, prefix);\n>  \trev.diffopt.context = 7;\n>\n> -\targc = setup_revisions(argc, argv, &rev, NULL);\n> +\tsetup_revisions(argc, argv, &rev, NULL);\n\nThis looks fishy.\n\nI guess this was in reaction to some compiler warning that said that\n`argc` is not used after it was assigned?\n\nIf that is the case, I would highly recommend against this hunk: the\n`setup_revisions()` function does alter the `argv` array, and `argc` is no\nlonger necessarily correct afterwards. Sure, if there is no _current_ user\nof `argc` later in the code, you could remove that assignment Right Now.\nBut future patches might need `argc` to be correct, and from experience I\ncan tell you that those kinds of lurking bugs are no fun to figure out at\nall.\n\nSo I'd say let's just drop this hunk.\n\n>  \trev.diffopt.output_format = DIFF_FORMAT_PATCH;\n>  \trev.diffopt.use_color = 0;\n>  \trev.diffopt.flags.ignore_dirty_submodules = 1;\n> @@ -486,6 +486,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n>  \t\t\tdie(_(\"--dry-run is incompatible with --interactive/--patch\"));\n>  \t\tif (pathspec_from_file)\n>  \t\t\tdie(_(\"--pathspec-from-file is incompatible with --interactive/--patch\"));\n> +\t\tif (edit_interactive)\n> +\t\t\tdie(_(\"--edit-interactive is incompatible with --interactive/--patch\"));\n\nThis hunk, in contrast, makes a lot of sense to me.\n\nBoth 1/2 and 2/2 (after dropping the first hunk of 2/2) are: `Acked-by`\nand/or `Reviewed-by` me, whichever you prefer.\n\nThank you,\nDscho\n\n>  \t\texit(interactive_add(argv + 1, prefix, patch_interactive));\n>  \t}\n>\n> --\n> 2.33.0.476.gf000ecbed9\n>\n>\n"},{"id":"434210","messageId":"YS146dlPrxwF0dl6@nand.local","threadId":"56303","inReplyTo":"20210817064435.97625-2-carenas@gmail.com","subject":"Re: [PATCH 1/2] builtin/add: remove obsoleted support for legacy stash -p","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-08-31T00:33:45Z","receivedAt":"2021-08-31T00:33:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 16, 2021 at 11:44:34PM -0700, Carlo Marcelo Arenas Belón wrote:\n> 90a6bb98d1 (legacy stash -p: respect the add.interactive.usebuiltin\n> setting, 2019-12-21) adds a hidden option and its supporting code\n> to support the legacy stash script, but that was left behind when\n> it was retired.\n>\n> mostly revert commit.\n\n(Sorry for a much-delayed response, I'm trying to do a little bit of\ninbox-cleaning ;)).\n\nIf you're re-rolling based on Dscho's suggestions later on in the\nseries, I'd suggest two changes to make the patch message clearer:\n\n  - Clarify the antecedent of \"it\" in \"it was retired\". I find that\n    \"...but that [option] was forgotten about even when [the legacy\n    stash implementation] was retired\".\n\n  - Link to the commit where we dropped support for that implementation\n    (which was in 8a2cd3f512 (stash: remove the stash.useBuiltin\n    setting, 2020-03-03)) to make it clear that that happened after\n    90a6bb98d1.\n\n  - Finally \"mostly revert commit.\" should add more detail without being\n    verbose. I'd write:\n\n        Since 8a2cd3f512 removed the legacy implementation, the changes\n        from 90a6bb98d1 are no longer necessary, so revert them.\n\n> @@ -483,6 +480,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n>  \t\t\t  builtin_add_usage, PARSE_OPT_KEEP_ARGV0);\n>  \tif (patch_interactive)\n>  \t\tadd_interactive = 1;\n> +\n\nStray whitespace change, but the rest of the patch looks good to me.\n\nThanks,\nTaylor\n"}]}