| Message ID | 20200826011718.3186597-4-gitster@pobox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | War on dashed-git | expand |
Junio C Hamano <gitster@pobox.com> writes: > If we were to propose removing "git-foo" binaries from the > filesystem for built-in commands, we should first see if there are > users who will be affected by such a move. When cmd_main() detects > we were called not as "git", but as "git-foo", give an error message > to ask the user to let us know and stop our removal plan, unless we > are running a selected few programs that MUST be callable in the > dashed form (e.g. "git-upload-pack"). > > Those who are always using "git foo" form will not be affected, but > those who trusted the promise we made to them 12 years ago that by > prepending the output of $(git --exec-path) to the list of > directories on their $PATH, they can safely keep writing > "git-cat-file" will be negatively affected as all their scripts > assuming the promise will be kept are now broken. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- The same idea as the one for pack-redundant. I do not use the technique to inspect $PATH and see if $GIT_EXEC_PATH is on it, as that would mean we will *not* bug users with legitimate need to keep the feature working, hence will not get "don't do that" objections. We may want to ensure command_list[] is sorted by name and run binary search on it if running find_cmdname_help() for each and every dashed "git-foo" invocations turns out to be costly. Our conjecture behind this patch is that the form is rarely if ever used, so it may not matter at all, though.
Hi Junio, On Tue, 25 Aug 2020, Junio C Hamano wrote: > If we were to propose removing "git-foo" binaries from the > filesystem for built-in commands, we should first see if there are > users who will be affected by such a move. When cmd_main() detects > we were called not as "git", but as "git-foo", give an error message > to ask the user to let us know and stop our removal plan, unless we > are running a selected few programs that MUST be callable in the > dashed form (e.g. "git-upload-pack"). > > Those who are always using "git foo" form will not be affected, but > those who trusted the promise we made to them 12 years ago that by > prepending the output of $(git --exec-path) to the list of > directories on their $PATH, they can safely keep writing > "git-cat-file" will be negatively affected as all their scripts > assuming the promise will be kept are now broken. It might be a good idea to also instrument the existing scripts, to show the same warning unless they were called through the `git` binary. _If_ we were to do this ;-) Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi Junio, > > On Tue, 25 Aug 2020, Junio C Hamano wrote: > >> If we were to propose removing "git-foo" binaries from the >> filesystem for built-in commands, we should first see if there are >> users who will be affected by such a move. When cmd_main() detects >> we were called not as "git", but as "git-foo", give an error message >> to ask the user to let us know and stop our removal plan, unless we >> are running a selected few programs that MUST be callable in the >> dashed form (e.g. "git-upload-pack"). >> >> Those who are always using "git foo" form will not be affected, but >> those who trusted the promise we made to them 12 years ago that by >> prepending the output of $(git --exec-path) to the list of >> directories on their $PATH, they can safely keep writing >> "git-cat-file" will be negatively affected as all their scripts >> assuming the promise will be kept are now broken. > > It might be a good idea to also instrument the existing scripts, to show > the same warning unless they were called through the `git` binary. > > _If_ we were to do this ;-) Sure. I am not the advocate for removing builtins from on-disk, though. The burden of proof... ;-)
Hi Junio, On Tue, 25 Aug 2020, Junio C Hamano wrote: > diff --git a/git.c b/git.c > index 8bd1d7551d..927018bda7 100644 > --- a/git.c > +++ b/git.c > @@ -839,6 +839,8 @@ int cmd_main(int argc, const char **argv) > * that one cannot handle it. > */ > if (skip_prefix(cmd, "git-", &cmd)) { > + warn_on_dashed_git(argv[0]); > + > argv[0] = cmd; > handle_builtin(argc, argv); > die(_("cannot handle %s as a builtin"), cmd); > diff --git a/help.c b/help.c > index d478afb2af..490d2bc3ae 100644 > --- a/help.c > +++ b/help.c > @@ -720,3 +720,37 @@ NORETURN void help_unknown_ref(const char *ref, const char *cmd, > string_list_clear(&suggested_refs, 0); > exit(1); > } > + > +static struct cmdname_help *find_cmdname_help(const char *name) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(command_list); i++) { > + if (!strcmp(command_list[i].name, name)) > + return &command_list[i]; > + } > + return NULL; > +} > + > +void warn_on_dashed_git(const char *cmd) > +{ > + struct cmdname_help *cmdname; > + static const char *still_in_use_var = "GIT_I_STILL_USE_DASHED_GIT"; > + static const char *still_in_use_msg = > + N_("Use of '%s' in the dashed-form is nominated for removal.\n" > + "If you still use it, export '%s=true'\n" > + "and send an e-mail to <git@vger.kernel.org>\n" > + "to let us know and stop our removal plan. Thanks.\n"); > + > + if (!cmd) > + return; /* git-help is OK */ > + > + cmdname = find_cmdname_help(cmd); > + if (cmdname && (cmdname->category & CAT_onpath)) > + return; /* git-upload-pack and friends are OK */ > + > + if (!git_env_bool(still_in_use_var, 0)) { > + fprintf(stderr, _(still_in_use_msg), cmd, still_in_use_var); > + exit(1); > + } > +} I need this on top, to make it work on Windows: -- snipsnap -- From: Johannes Schindelin <johannes.schindelin@gmx.de> Subject: [PATCH] fixup??? git: catch an attempt to run "git-foo" This is needed to handle the case where `argv[0]` contains the full path (which is the case on Windows) and the suffix `.exe` (which is also the case on Windows). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- git.c | 3 ++- help.c | 5 ++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/git.c b/git.c index 71ef4835b20e..863fd0c58a66 100644 --- a/git.c +++ b/git.c @@ -851,7 +851,8 @@ int cmd_main(int argc, const char **argv) * that one cannot handle it. */ if (skip_prefix(cmd, "git-", &cmd)) { - warn_on_dashed_git(argv[0]); + strip_extension(&cmd); + warn_on_dashed_git(cmd); argv[0] = cmd; handle_builtin(argc, argv); diff --git a/help.c b/help.c index c93a76944b00..27b1b26890be 100644 --- a/help.c +++ b/help.c @@ -724,9 +724,12 @@ NORETURN void help_unknown_ref(const char *ref, const char *cmd, static struct cmdname_help *find_cmdname_help(const char *name) { int i; + const char *p; + skip_prefix(name, "git-", &name); for (i = 0; i < ARRAY_SIZE(command_list); i++) { - if (!strcmp(command_list[i].name, name)) + if (skip_prefix(command_list[i].name, "git-", &p) && + !strcmp(p, name)) return &command_list[i]; } return NULL; -- 2.28.0.windows.1
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > This is needed to handle the case where `argv[0]` contains the full path > (which is the case on Windows) and the suffix `.exe` (which is also the > case on Windows). Oy. Yeah, I totally forgot about ".exe" thing. > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > git.c | 3 ++- > help.c | 5 ++++- > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/git.c b/git.c > index 71ef4835b20e..863fd0c58a66 100644 > --- a/git.c > +++ b/git.c > @@ -851,7 +851,8 @@ int cmd_main(int argc, const char **argv) > * that one cannot handle it. > */ > if (skip_prefix(cmd, "git-", &cmd)) { > - warn_on_dashed_git(argv[0]); > + strip_extension(&cmd); > + warn_on_dashed_git(cmd); The argv[0] may have been NULL from the beginning of cmd_main(), and cmd would be "git-help" in such a case. We used to pass NULL to warn_on_dashed_git() in such a case because "git-help" is not what the user typed, but what we internally trigger, so we didn't want warn_on_dashed_git() to do anything based on that internal string. As there is no special casing of "help" in warn_on_dashed_git() mechanism, it probably would start triggering a warning/die when "git<ENTER>" is typed, no? + if (argv[0]) { strip_extension(&cmd); warn_on_dashed_git(cmd); may be the minimum fix, but I would strongly prefer to keep the interface into warn_on_dashed_git() (eh, the most important is the interface into find_cmdname_help() helper function, which is designed to be reused by other parts of help.c) to take the full command name, not without "git-" prefix. This is primarily because the entirety of the help.c API is driven by full command names, without removing "git-" prefix, and it has to, because the help.c API needs to handle "gitk", from which you cannot remove any "git-" prefix. Perhaps if (starts_with(cmd, "git-")) { strip_extension(&cmd); if (argv[0]) warn_on_dashed_git(cmd); argv[0] = cmd + 4; handle_builtin(argc, argv); die(...); How does your handle_builtin() work, by the way? The original code (even before we added warn_on_dashed_git() in this codepath) did not do any strip_extension(), so handle_builtin() can take commands with ".exe" suffix, but now we are feeding the result of strip_extension() to it, so it can deal with both? Sounds convenient and sloppy (not the handle_builtin's implementation, but its callers that sometimes feeds the full executable name, and feeds only the basename some other times) at the same time. > argv[0] = cmd; > handle_builtin(argc, argv); > diff --git a/help.c b/help.c > index c93a76944b00..27b1b26890be 100644 > --- a/help.c > +++ b/help.c > @@ -724,9 +724,12 @@ NORETURN void help_unknown_ref(const char *ref, const char *cmd, > static struct cmdname_help *find_cmdname_help(const char *name) > { > int i; > + const char *p; > > + skip_prefix(name, "git-", &name); > for (i = 0; i < ARRAY_SIZE(command_list); i++) { > - if (!strcmp(command_list[i].name, name)) > + if (skip_prefix(command_list[i].name, "git-", &p) && > + !strcmp(p, name)) > return &command_list[i]; > } > return NULL; > -- > 2.28.0.windows.1
Hi Junio, On Fri, 28 Aug 2020, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > git.c | 3 ++- > > help.c | 5 ++++- > > 2 files changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/git.c b/git.c > > index 71ef4835b20e..863fd0c58a66 100644 > > --- a/git.c > > +++ b/git.c > > @@ -851,7 +851,8 @@ int cmd_main(int argc, const char **argv) > > * that one cannot handle it. > > */ > > if (skip_prefix(cmd, "git-", &cmd)) { > > - warn_on_dashed_git(argv[0]); > > + strip_extension(&cmd); > > + warn_on_dashed_git(cmd); > > The argv[0] may have been NULL from the beginning of cmd_main(), and > cmd would be "git-help" in such a case. We used to pass NULL to > warn_on_dashed_git() in such a case because "git-help" is not what the > user typed, but what we internally trigger, so we didn't want > warn_on_dashed_git() to do anything based on that internal string. True. The test suite passes with this, though, which means we haven't covered that case. > As there is no special casing of "help" in warn_on_dashed_git() > mechanism, it probably would start triggering a warning/die when > "git<ENTER>" is typed, no? > > + if (argv[0]) { > strip_extension(&cmd); > warn_on_dashed_git(cmd); > > may be the minimum fix, but I would strongly prefer to keep the > interface into warn_on_dashed_git() (eh, the most important is the > interface into find_cmdname_help() helper function, which is > designed to be reused by other parts of help.c) to take the full > command name, not without "git-" prefix. This is primarily because > the entirety of the help.c API is driven by full command names, > without removing "git-" prefix, and it has to, because the help.c > API needs to handle "gitk", from which you cannot remove any "git-" > prefix. > > Perhaps > > if (starts_with(cmd, "git-")) { > strip_extension(&cmd); > if (argv[0]) > warn_on_dashed_git(cmd); > argv[0] = cmd + 4; > handle_builtin(argc, argv); > die(...); Sure. > How does your handle_builtin() work, by the way? > > The original code (even before we added warn_on_dashed_git() in this > codepath) did not do any strip_extension(), so handle_builtin() can > take commands with ".exe" suffix, but now we are feeding the result > of strip_extension() to it, so it can deal with both? Yes, it can deal with both. It calls `strip_extension()`, which on Windows removes the `.exe` suffix, if found. > Sounds convenient and sloppy (not the handle_builtin's > implementation, but its callers that sometimes feeds the full > executable name, and feeds only the basename some other times) at > the same time. Right, it does not _require_ the extension. I do not necessarily agree that it is sloppy, but I do agree that it is convenient ;-) Ciao, Dscho > > argv[0] = cmd; > > handle_builtin(argc, argv); > > diff --git a/help.c b/help.c > > index c93a76944b00..27b1b26890be 100644 > > --- a/help.c > > +++ b/help.c > > @@ -724,9 +724,12 @@ NORETURN void help_unknown_ref(const char *ref, const char *cmd, > > static struct cmdname_help *find_cmdname_help(const char *name) > > { > > int i; > > + const char *p; > > > > + skip_prefix(name, "git-", &name); > > for (i = 0; i < ARRAY_SIZE(command_list); i++) { > > - if (!strcmp(command_list[i].name, name)) > > + if (skip_prefix(command_list[i].name, "git-", &p) && > > + !strcmp(p, name)) > > return &command_list[i]; > > } > > return NULL; > > -- > > 2.28.0.windows.1 >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi Junio, > > On Fri, 28 Aug 2020, Junio C Hamano wrote: > >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> >> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> >> > --- >> > git.c | 3 ++- >> > help.c | 5 ++++- >> > 2 files changed, 6 insertions(+), 2 deletions(-) >> > >> > diff --git a/git.c b/git.c >> > index 71ef4835b20e..863fd0c58a66 100644 >> > --- a/git.c >> > +++ b/git.c >> > @@ -851,7 +851,8 @@ int cmd_main(int argc, const char **argv) >> > * that one cannot handle it. >> > */ >> > if (skip_prefix(cmd, "git-", &cmd)) { >> > - warn_on_dashed_git(argv[0]); >> > + strip_extension(&cmd); >> > + warn_on_dashed_git(cmd); >> >> The argv[0] may have been NULL from the beginning of cmd_main(), and >> cmd would be "git-help" in such a case. We used to pass NULL to >> warn_on_dashed_git() in such a case because "git-help" is not what the >> user typed, but what we internally trigger, so we didn't want >> warn_on_dashed_git() to do anything based on that internal string. > > True. The test suite passes with this, though, which means we haven't > covered that case. Yup, it would be a good thing to add to our tests, with or without this patch. >> How does your handle_builtin() work, by the way? >> >> The original code (even before we added warn_on_dashed_git() in this >> codepath) did not do any strip_extension(), so handle_builtin() can >> take commands with ".exe" suffix, but now we are feeding the result >> of strip_extension() to it, so it can deal with both? > > Yes, it can deal with both. It calls `strip_extension()`, which on Windows > removes the `.exe` suffix, if found. > >> Sounds convenient and sloppy (not the handle_builtin's >> implementation, but its callers that sometimes feeds the full >> executable name, and feeds only the basename some other times) at >> the same time. > > Right, it does not _require_ the extension. I do not necessarily agree > that it is sloppy, but I do agree that it is convenient ;-) Being lenient to its input is not sloppy. It just is by being convenient, it allows its callers to be sloppy, which may hurt them as not all callees they call may not be as lenient as handle_builtin(), which is the only downside I would be worried about. Nothing big. thanks.
Hi Junio, On Fri, 28 Aug 2020, Johannes Schindelin wrote: > On Tue, 25 Aug 2020, Junio C Hamano wrote: > > > diff --git a/git.c b/git.c > > index 8bd1d7551d..927018bda7 100644 > > --- a/git.c > > +++ b/git.c > > @@ -839,6 +839,8 @@ int cmd_main(int argc, const char **argv) > > * that one cannot handle it. > > */ > > if (skip_prefix(cmd, "git-", &cmd)) { > > + warn_on_dashed_git(argv[0]); > > + > > argv[0] = cmd; > > handle_builtin(argc, argv); > > die(_("cannot handle %s as a builtin"), cmd); > > diff --git a/help.c b/help.c > > index d478afb2af..490d2bc3ae 100644 > > --- a/help.c > > +++ b/help.c > > @@ -720,3 +720,37 @@ NORETURN void help_unknown_ref(const char *ref, const char *cmd, > > string_list_clear(&suggested_refs, 0); > > exit(1); > > } > > + > > +static struct cmdname_help *find_cmdname_help(const char *name) > > +{ > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(command_list); i++) { > > + if (!strcmp(command_list[i].name, name)) > > + return &command_list[i]; > > + } > > + return NULL; > > +} > > + > > +void warn_on_dashed_git(const char *cmd) > > +{ > > + struct cmdname_help *cmdname; > > + static const char *still_in_use_var = "GIT_I_STILL_USE_DASHED_GIT"; > > + static const char *still_in_use_msg = > > + N_("Use of '%s' in the dashed-form is nominated for removal.\n" > > + "If you still use it, export '%s=true'\n" > > + "and send an e-mail to <git@vger.kernel.org>\n" > > + "to let us know and stop our removal plan. Thanks.\n"); > > + > > + if (!cmd) > > + return; /* git-help is OK */ > > + > > + cmdname = find_cmdname_help(cmd); > > + if (cmdname && (cmdname->category & CAT_onpath)) > > + return; /* git-upload-pack and friends are OK */ > > + > > + if (!git_env_bool(still_in_use_var, 0)) { > > + fprintf(stderr, _(still_in_use_msg), cmd, still_in_use_var); > > + exit(1); > > + } > > +} > > I need this on top, to make it work on Windows: > > -- snipsnap -- > From: Johannes Schindelin <johannes.schindelin@gmx.de> > Subject: [PATCH] fixup??? git: catch an attempt to run "git-foo" > > This is needed to handle the case where `argv[0]` contains the full path > (which is the case on Windows) and the suffix `.exe` (which is also the > case on Windows). > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > git.c | 3 ++- > help.c | 5 ++++- > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/git.c b/git.c > index 71ef4835b20e..863fd0c58a66 100644 > --- a/git.c > +++ b/git.c > @@ -851,7 +851,8 @@ int cmd_main(int argc, const char **argv) > * that one cannot handle it. > */ > if (skip_prefix(cmd, "git-", &cmd)) { > - warn_on_dashed_git(argv[0]); > + strip_extension(&cmd); > + warn_on_dashed_git(cmd); > > argv[0] = cmd; > handle_builtin(argc, argv); > diff --git a/help.c b/help.c > index c93a76944b00..27b1b26890be 100644 > --- a/help.c > +++ b/help.c > @@ -724,9 +724,12 @@ NORETURN void help_unknown_ref(const char *ref, const char *cmd, > static struct cmdname_help *find_cmdname_help(const char *name) > { > int i; > + const char *p; > > + skip_prefix(name, "git-", &name); > for (i = 0; i < ARRAY_SIZE(command_list); i++) { > - if (!strcmp(command_list[i].name, name)) > + if (skip_prefix(command_list[i].name, "git-", &p) && > + !strcmp(p, name)) > return &command_list[i]; > } > return NULL; > -- > 2.28.0.windows.1 How about this instead (to fix that part of the CI failures of `seen`)? -- snipsnap -- From e8ce19db04657b6ef1c73989695c97a773a9c001 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin <johannes.schindelin@gmx.de> Date: Fri, 28 Aug 2020 14:50:25 +0200 Subject: [PATCH] fixup??? git: catch an attempt to run "git-foo" This is needed to handle the case where `argv[0]` contains the full path (which is the case on Windows) and the suffix `.exe` (which is also the case on Windows). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- git.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/git.c b/git.c index 7544d2187306..c924c53ea76f 100644 --- a/git.c +++ b/git.c @@ -854,6 +854,7 @@ int cmd_main(int argc, const char **argv) const char *cmd; int done_help = 0; + strip_extension(argv); cmd = argv[0]; if (!cmd) cmd = "git-help"; @@ -875,12 +876,11 @@ int cmd_main(int argc, const char **argv) * So we just directly call the builtin handler, and die if * that one cannot handle it. */ - if (skip_prefix(cmd, "git-", &cmd)) { - warn_on_dashed_git(argv[0]); + if (skip_prefix(cmd, "git-", &argv[0])) { + warn_on_dashed_git(cmd); - argv[0] = cmd; handle_builtin(argc, argv); - die(_("cannot handle %s as a builtin"), cmd); + die(_("cannot handle %s as a builtin"), argv[0]); } /* Look for flags.. */ -- 2.30.0.rc0.windows.1
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> I need this on top, to make it work on Windows: >> ... > How about this instead (to fix that part of the CI failures of `seen`)? Ah, I didn't knew the backburnered stuff was breaking 'seen'. Thanks for helping to cleanse 'seen'; I do not actually mind dropping the offending topic at this point in the cycle, though. > -- snipsnap -- > From e8ce19db04657b6ef1c73989695c97a773a9c001 Mon Sep 17 00:00:00 2001 > From: Johannes Schindelin <johannes.schindelin@gmx.de> > Date: Fri, 28 Aug 2020 14:50:25 +0200 > Subject: [PATCH] fixup??? git: catch an attempt to run "git-foo" > > This is needed to handle the case where `argv[0]` contains the full path > (which is the case on Windows) and the suffix `.exe` (which is also the > case on Windows). > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > git.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/git.c b/git.c > index 7544d2187306..c924c53ea76f 100644 > --- a/git.c > +++ b/git.c > @@ -854,6 +854,7 @@ int cmd_main(int argc, const char **argv) > const char *cmd; > int done_help = 0; > > + strip_extension(argv); > cmd = argv[0]; Hph, would this make strip_extension() at the beginning of handle_builtin() redundant and unneeded, I wonder? Yes, I know stripping .exe twice would be fine most of the time, so I'll queue the patch on top just to make 'seen' pass the tests, but it is just as easy to discard jc/war-on-dashed-git topic, so... > if (!cmd) > cmd = "git-help"; > @@ -875,12 +876,11 @@ int cmd_main(int argc, const char **argv) > * So we just directly call the builtin handler, and die if > * that one cannot handle it. > */ > - if (skip_prefix(cmd, "git-", &cmd)) { > - warn_on_dashed_git(argv[0]); > + if (skip_prefix(cmd, "git-", &argv[0])) { > + warn_on_dashed_git(cmd); > > - argv[0] = cmd; > handle_builtin(argc, argv); > - die(_("cannot handle %s as a builtin"), cmd); > + die(_("cannot handle %s as a builtin"), argv[0]); > } > > /* Look for flags.. */ > -- > 2.30.0.rc0.windows.1
Hi Junio, On Mon, 21 Dec 2020, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > -- snipsnap -- > > From e8ce19db04657b6ef1c73989695c97a773a9c001 Mon Sep 17 00:00:00 2001 > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > Date: Fri, 28 Aug 2020 14:50:25 +0200 > > Subject: [PATCH] fixup??? git: catch an attempt to run "git-foo" > > > > This is needed to handle the case where `argv[0]` contains the full path > > (which is the case on Windows) and the suffix `.exe` (which is also the > > case on Windows). > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > git.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/git.c b/git.c > > index 7544d2187306..c924c53ea76f 100644 > > --- a/git.c > > +++ b/git.c > > @@ -854,6 +854,7 @@ int cmd_main(int argc, const char **argv) > > const char *cmd; > > int done_help = 0; > > > > + strip_extension(argv); > > cmd = argv[0]; > > Hph, would this make strip_extension() at the beginning of > handle_builtin() redundant and unneeded, I wonder? I poured over this for a couple minutes, and I think you're right. > Yes, I know stripping .exe twice would be fine most of the time, so > I'll queue the patch on top just to make 'seen' pass the tests, but > it is just as easy to discard jc/war-on-dashed-git topic, so... I dunno. There is probably value in starting the deprecation for realz. I mean, we deprecated dashed commands, like, an age ago (2020 alone felt like a century, didn't it?). Maybe it is time to warn about using dashed commands now. We could even consider "brown-outs" at some stage, via a pseudo-random condition that triggers rarely in spring 2021 but incrementally frequently over time? Ciao, Dscho
diff --git a/command-list.txt b/command-list.txt index e5901f2213..1238f6ec8b 100644 --- a/command-list.txt +++ b/command-list.txt @@ -39,6 +39,9 @@ # mainporcelain commands are completable so you don't need this # attribute. # +# "onpath" attribute is used to mark that the command MUST appear +# on $PATH (typically in /usr/bin) due to protocol requirement. +# # As part of the Git man page list, the man(5/7) guides are also # specified here, which can only have "guide" attribute and nothing # else. @@ -144,7 +147,7 @@ git-quiltimport foreignscminterface git-range-diff mainporcelain git-read-tree plumbingmanipulators git-rebase mainporcelain history -git-receive-pack synchelpers +git-receive-pack synchelpers onpath git-reflog ancillarymanipulators complete git-remote ancillarymanipulators complete git-repack ancillarymanipulators complete @@ -159,7 +162,7 @@ git-rev-parse plumbinginterrogators git-rm mainporcelain worktree git-send-email foreignscminterface complete git-send-pack synchingrepositories -git-shell synchelpers +git-shell synchelpers onpath git-shortlog mainporcelain git-show mainporcelain info git-show-branch ancillaryinterrogators complete @@ -182,8 +185,8 @@ git-unpack-objects plumbingmanipulators git-update-index plumbingmanipulators git-update-ref plumbingmanipulators git-update-server-info synchingrepositories -git-upload-archive synchelpers -git-upload-pack synchelpers +git-upload-archive synchelpers onpath +git-upload-pack synchelpers onpath git-var plumbinginterrogators git-verify-commit ancillaryinterrogators git-verify-pack plumbinginterrogators diff --git a/git.c b/git.c index 8bd1d7551d..927018bda7 100644 --- a/git.c +++ b/git.c @@ -839,6 +839,8 @@ int cmd_main(int argc, const char **argv) * that one cannot handle it. */ if (skip_prefix(cmd, "git-", &cmd)) { + warn_on_dashed_git(argv[0]); + argv[0] = cmd; handle_builtin(argc, argv); die(_("cannot handle %s as a builtin"), cmd); diff --git a/help.c b/help.c index d478afb2af..490d2bc3ae 100644 --- a/help.c +++ b/help.c @@ -720,3 +720,37 @@ NORETURN void help_unknown_ref(const char *ref, const char *cmd, string_list_clear(&suggested_refs, 0); exit(1); } + +static struct cmdname_help *find_cmdname_help(const char *name) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(command_list); i++) { + if (!strcmp(command_list[i].name, name)) + return &command_list[i]; + } + return NULL; +} + +void warn_on_dashed_git(const char *cmd) +{ + struct cmdname_help *cmdname; + static const char *still_in_use_var = "GIT_I_STILL_USE_DASHED_GIT"; + static const char *still_in_use_msg = + N_("Use of '%s' in the dashed-form is nominated for removal.\n" + "If you still use it, export '%s=true'\n" + "and send an e-mail to <git@vger.kernel.org>\n" + "to let us know and stop our removal plan. Thanks.\n"); + + if (!cmd) + return; /* git-help is OK */ + + cmdname = find_cmdname_help(cmd); + if (cmdname && (cmdname->category & CAT_onpath)) + return; /* git-upload-pack and friends are OK */ + + if (!git_env_bool(still_in_use_var, 0)) { + fprintf(stderr, _(still_in_use_msg), cmd, still_in_use_var); + exit(1); + } +} diff --git a/help.h b/help.h index dc02458855..d3de5e0d69 100644 --- a/help.h +++ b/help.h @@ -45,6 +45,9 @@ void get_version_info(struct strbuf *buf, int show_build_options); */ NORETURN void help_unknown_ref(const char *ref, const char *cmd, const char *error); +/* When the cmd_main() sees "git-foo", check if the user intended */ +void warn_on_dashed_git(const char *); + static inline void list_config_item(struct string_list *list, const char *prefix, const char *str)
If we were to propose removing "git-foo" binaries from the filesystem for built-in commands, we should first see if there are users who will be affected by such a move. When cmd_main() detects we were called not as "git", but as "git-foo", give an error message to ask the user to let us know and stop our removal plan, unless we are running a selected few programs that MUST be callable in the dashed form (e.g. "git-upload-pack"). Those who are always using "git foo" form will not be affected, but those who trusted the promise we made to them 12 years ago that by prepending the output of $(git --exec-path) to the list of directories on their $PATH, they can safely keep writing "git-cat-file" will be negatively affected as all their scripts assuming the promise will be kept are now broken. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- command-list.txt | 11 +++++++---- git.c | 2 ++ help.c | 34 ++++++++++++++++++++++++++++++++++ help.h | 3 +++ 4 files changed, 46 insertions(+), 4 deletions(-)