Re: [PATCH v4] git{,-blame}.el: remove old bitrotting Emacs code

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 12, 2018, 06:52 UTC
Message-ID: <87fu40c3xe.fsf@evledraar.gmail.com>
In-Reply-To: <xmqqk1td2ml2.fsf@gitster-ct.c.googlers.com>

On Thu, Apr 12 2018, Junio C. Hamano wrote:

Show 49 quoted lines

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> However, since downstream packagers such as Debian are packaging this
>> as git-el it's less disruptive to still carry these files as Elisp
>> code that'll error out with a message suggesting alternatives, rather
>> than drop the files entirely[2].
>>
>> Then rather than receive a cryptic load error when they upgrade
>> existing users will get an error directing them to the README file, or
>> to just stop requiring these modes. I think it makes sense to link to
>> GitHub's hosting of contrib/emacs/README (which'll be updated by the
>> time users see this) so they don't have to hunt down the packaged
>> README on their local system.
>> ...
>>
>>  contrib/emacs/.gitignore   |    1 -
>>  contrib/emacs/Makefile     |   21 -
>>  contrib/emacs/README       |   32 +-
>>  contrib/emacs/git-blame.el |  489 +----------
>>  contrib/emacs/git.el       | 1710 +-----------------------------------
>>  5 files changed, 25 insertions(+), 2228 deletions(-)
>>  delete mode 100644 contrib/emacs/.gitignore
>>  delete mode 100644 contrib/emacs/Makefile
>
> I know I am to blame for prodding you to reopen this topic, but I am
> wondering if removal of Makefile is sensible.  Is the assumption
> that the distro packagers won't be using this Makefile at all and
> have their own (e.g. debian/rules for Debian) build procedure, hence
> *.el files are all they need to have?
>
> The reason why I am wondering is because I do not know what distro
> folks would do when they find that their build procedure does not
> work---I suspect the would punt, and end users of the distro would
> find that git-el package is no longer with them.  These end users
> are whom this discussion is trying to help, but then to these
> packagers, the reason why their build procedure no longer works does
> not really matter, whether git.el is not shipped, or Makefile that
> their debian/rules-equivalent depends on is not there, for them to
> decide dropping the git-el package.
>
> On the other hand, the 6-lines of e-lisp you wrote for git.el
> replacement is something the packagers could have written for their
> users, so (1) if we really want to go extra mile without trusting
> that distro packagers are less competent than us in helping their
> users, we'd be better off to leave Makefile in, or (2) if we trust
> packagers and leave possible end-user confusion as their problem
> (not ours), then we can just remove as your previous round did.
>
> And from that point of view, I find this round slightly odd.

I think the way it is makes sense. In Debian debian/git-el.install just does:

    contrib/emacs/git-blame.el usr/share/git-core/emacs
    contrib/emacs/git.el usr/share/git-core/emacs

Which means on installation they don't even use our contrib/emacs/Makefile, but instead on installation do:

    Setting up git-el (1:2.16.3-1) ...
    Install git for emacs
    Install git for emacs24
    install/git: Handling install of emacsen flavor emacs24
    install/git: Byte-compiling for emacs24
    + emacs24 -batch -q -no-site-file -f batch-byte-compile git.el git-blame.el
    Wrote /usr/share/emacs24/site-lisp/git/git.elc
    Wrote /usr/share/emacs24/site-lisp/git/git-blame.elc
    Install git for emacs25
    install/git: Handling install of emacsen flavor emacs25
    install/git: Byte-compiling for emacs25
    + emacs25 -batch -q -no-site-file -f batch-byte-compile git.el git-blame.el

RedHat does use contrib/emacs/Makefile:

    https://src.fedoraproject.org/cgit/rpms/git.git/tree/git.spec#n493

But they can either just do their own byte compilation as they surely do for other elisp packages, or just do this:

     git-init.el | 5 -----
     git.spec    | 9 ++-------
     2 files changed, 2 insertions(+), 12 deletions(-)

    diff --git a/git-init.el b/git-init.el
    deleted file mode 100644
    index d2a96a7..0000000
    --- a/git-init.el
    +++ /dev/null
    @@ -1,5 +0,0 @@
    -;; Git VC backend
    -(add-to-list 'vc-handled-backends 'GIT t)
    -(autoload 'git-status "git" "GIT mode." t)
    -(autoload 'git-blame-mode "git-blame"
    -       "Minor mode for incremental blame for Git." t)
    diff --git a/git.spec b/git.spec
    index ee60d3e..a82c1aa 100644
    --- a/git.spec
    +++ b/git.spec
    @@ -87,7 +87,6 @@ Source1:        https://www.kernel.org/pub/software/scm/git/%{?rcrev:testing/}%{
     Source9:        gpgkey-junio.asc

     # Local sources begin at 10 to allow for additional future upstream sources
    -Source10:       git-init.el
     Source11:       git.xinetd.in
     Source12:       git-gui.desktop
     Source13:       gitweb-httpd.conf
    @@ -491,14 +490,10 @@ for i in git git-shell git-upload-pack; do
     done

     %global elispdir %{_emacs_sitelispdir}/git
    -make -C contrib/emacs install \
    -    emacsdir=%{buildroot}%{elispdir}
    -for elc in %{buildroot}%{elispdir}/*.elc ; do
    -    install -pm 644 contrib/emacs/$(basename $elc .elc).el \
    +for el in %{buildroot}%{elispdir}/*.el ; do
    +    install -pm 644 contrib/emacs/$elc \
         %{buildroot}%{elispdir}
     done
    -install -Dpm 644 %{SOURCE10} \
    -    %{buildroot}%{_emacs_sitestartdir}/git-init.el

     %if %{libsecret}
     install -pm 755 contrib/credential/libsecret/git-credential-libsecret \

I.e. there's no rule that there *must* be an *.elc file, it's just a generally good idea, but if your file is purely a one-line (error) there's no point in pre-compiling it.