| Message ID | cover-0.9-00000000000-20210902T155758Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | remove dead shell code | expand |
On Thu, Sep 2, 2021 at 6:04 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > Remove dead shell code in git-sh-setup, inspired by parallel > discussion on another topic (but the two don't conflict): > https://lore.kernel.org/git/87lf4f9gre.fsf@evledraar.gmail.com/ > > The last two patches were picked from a dropped series of mine > submitted earlier this year, it was dropped because of other more > complex patches that I haven't included here: > https://lore.kernel.org/git/20210311001447.28254-1-avarab@gmail.com/ > [...] Hm, I have scripts here, implementing some porcelain commands which follow the same approach as the git porcelain scripts, e.g. #!/bin/bash SUBDIRECTORY_OK=Yes . git-sh-setup require_work_tree require_clean_work_tree cd_to_toplevel || die "Can't find top level for the git repo" set_reflog_action my-special-script # this will be broken by the patch series I was under the impression that this is how it should be done when one needs to write some custom git scripts. If that is the case, then removing these functions might break some other users scripts out there. At least I have a quite an old self written script here which uses `set_reflog_action`. Obviously I can manage and adapt the script, but the more generic question comes to mind if sourcing git-sh-setup by end users is the correct approach to write porcelain scripts! -Peter
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Remove dead shell code in git-sh-setup, inspired by parallel > discussion on another topic (but the two don't conflict): > https://lore.kernel.org/git/87lf4f9gre.fsf@evledraar.gmail.com/ > > The last two patches were picked from a dropped series of mine > submitted earlier this year, it was dropped because of other more > complex patches that I haven't included here: > https://lore.kernel.org/git/20210311001447.28254-1-avarab@gmail.com/ > > Ævar Arnfjörð Bjarmason (9): > git-sh-setup: remove unused set_reflog_action() function > git-sh-setup: remove unused git_editor() function > git-sh-setup: remove unused git_pager() function > git-sh-setup: remove unused sane_egrep() function > git-sh-setup: remove unused require_work_tree_exists() function > git-sh-setup: move create_virtual_base() to mergetools/p4merge > git-sh-setup: move peel_committish() function to git-subtree.sh > git-bisect: remove unused SHA-1 $x40 shell variable > test-lib: remove unused $_x40 and $_z40 variables Was "unused" above decided based solely on the presence of in-tree users? If that is the case, I do not think we want to take these sh-setup changes. The implementation details of the remaining part of git-bisect.sh and test-lib.sh are OK, of course, as that is truly our local issue. Thanks.
Peter Baumann <peter.baumann@gmail.com> writes: > Hm, I have scripts here, implementing some porcelain commands which > follow the same approach as > the git porcelain scripts, e.g. > > #!/bin/bash > SUBDIRECTORY_OK=Yes > . git-sh-setup > require_work_tree > require_clean_work_tree > cd_to_toplevel || die "Can't find top level for the git repo" > set_reflog_action my-special-script # this > will be broken by the patch series > > I was under the impression that this is how it should be done when one > needs to write some custom git scripts. The reason why output from "git log --stat -- git-sh-setup.sh" does not have that much removal is exactly this. These are part of our published API. It is very much appreciated that you raised this point. Thanks.
On Thu, Sep 2, 2021 at 1:58 PM Junio C Hamano <gitster@pobox.com> wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > > Ævar Arnfjörð Bjarmason (9): > > git-sh-setup: remove unused set_reflog_action() function > > git-sh-setup: remove unused git_editor() function > > git-sh-setup: remove unused git_pager() function > > git-sh-setup: remove unused sane_egrep() function > > git-sh-setup: remove unused require_work_tree_exists() function > > git-sh-setup: move create_virtual_base() to mergetools/p4merge > > git-sh-setup: move peel_committish() function to git-subtree.sh > > git-bisect: remove unused SHA-1 $x40 shell variable > > test-lib: remove unused $_x40 and $_z40 variables > > Was "unused" above decided based solely on the presence of in-tree > users? If that is the case, I do not think we want to take these > sh-setup changes. > > The implementation details of the remaining part of git-bisect.sh > and test-lib.sh are OK, of course, as that is truly our local > issue. The removal of sane_egrep() is also unlikely to cause issues, as it wasn't documented as a public API, and it seems similar to the git-bisect's implementation details you refer to, except for git-submodule. Dropping it now would avoid having to change it to `grep -E` as egrep gets obsoleted. In that line, git_pager() and peel_committish() aren't documented either; document or drop? Carlo
On Thu, Sep 02 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Remove dead shell code in git-sh-setup, inspired by parallel >> discussion on another topic (but the two don't conflict): >> https://lore.kernel.org/git/87lf4f9gre.fsf@evledraar.gmail.com/ >> >> The last two patches were picked from a dropped series of mine >> submitted earlier this year, it was dropped because of other more >> complex patches that I haven't included here: >> https://lore.kernel.org/git/20210311001447.28254-1-avarab@gmail.com/ >> >> Ævar Arnfjörð Bjarmason (9): >> git-sh-setup: remove unused set_reflog_action() function >> git-sh-setup: remove unused git_editor() function >> git-sh-setup: remove unused git_pager() function >> git-sh-setup: remove unused sane_egrep() function >> git-sh-setup: remove unused require_work_tree_exists() function >> git-sh-setup: move create_virtual_base() to mergetools/p4merge >> git-sh-setup: move peel_committish() function to git-subtree.sh >> git-bisect: remove unused SHA-1 $x40 shell variable >> test-lib: remove unused $_x40 and $_z40 variables > > Was "unused" above decided based solely on the presence of in-tree > users? If that is the case, I do not think we want to take these > sh-setup changes. I should have remembered to reference the earlier discussion, but I think we had this exact discussion around a year ago when I submitted patches to remove git-parse-remote.sh, and decided this direction was OK. See a89a2fbfccd (parse-remote: remove this now-unused library, 2020-11-14) and the thread starting at <20201111173738.GB9902@coredump.intra.peff.net>: https://lore.kernel.org/git/20201111173738.GB9902@coredump.intra.peff.net/ You'll know better what you meant, but I interpreted the docs you added for git-sh-setup in 850844e28f7 (Documentation/git-sh-setup.txt: programmer's docs, 2007-01-17) as a guide for in-tree porcelain scripts. As noted in my recently sent <87lf4f9gre.fsf@evledraar.gmail.com> (https://lore.kernel.org/git/87lf4f9gre.fsf@evledraar.gmail.com/) the eventual goal I have in mind here is to get rid of git-sh-i18n.sh. If we're set on maintaining these shell libraries indefinitely even after in-tree users have gone away that pretty much means we can't do that, which would be unfortunate. We continue paying for quite a bit of technical debt to extend certain parts of core C git functionality to *.sh and *.perl.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > You'll know better what you meant, but I interpreted the docs you added > for git-sh-setup in 850844e28f7 (Documentation/git-sh-setup.txt: > programmer's docs, 2007-01-17) as a guide for in-tree porcelain scripts. No, it is not for "in-tree". Especially back then, one of the Git's goal was to be and to stay scriptable, and git-sh-setup was for those who are scripting, either "in-tree" or custom Porcelain people wrote around Git as part of a larger Git ecosystem.
Carlo Arenas <carenas@gmail.com> writes: > Dropping it now would avoid having to change it to `grep -E` as egrep > gets obsoleted. Keeping it would isolate the callers from "grep -E" vs "egrep" because they do not need to know. They can keep calling sane_egrep(). Having said that, I do not think anybody minds losing sane_egrep(). Many helper functions in the shell library are about encapsulating the knowledge we have on and around Git and making it easier for third-party script writers to use the knowledge. The functions like is_bare_repository(), cd_to_toplevel(), and git_pager() all fall into that category. sane_egrep is not that kind of a helper (it encapusulates our knowledge about GNU grep's quirk we found when we tried to use it in our scripts---third-party script writers do not need help from Git experts on their use of egrep). Thanks.