Re: [elpa] master 872014e: Prevent accidental deletion of .git

[Thomas Fitzsimmons]

Stefan Monnier <address@hidden> writes:

>>> We'd also want to try and fix the directory-deletion code accordingly,
>>> so it's less trigger happy.
>> I've been looking at how to do this, but I'm not sure exactly what
>> you're after.  The following is from the perspective of "in place"
>> development.
>
> Yes, that's the important case.

BTW, I found myself being more focused on the "in place archive" use
case.  That's how I ended up invoking the process-archive target
directly which indirectly deleted .git (this is the one my patch now
prevents).

I think the default "make" target should also generate the archive
directory so that one can always test "what will be published" with:

rm -rf test-home && mkdir test-home && HOME=`pwd`/test-home $TARGET -Q \
    --eval \
    "(setq package-archives '((\"local\" . \".../elpa/archive/packages\")))"

Then one can test dependency resolution, byte-compilation and
compatibility on the "target Emacs" -- I test Emacs 24.1 through 24.5
and master.  Testing just against the default emacs on PATH (what "make"
does right now) is OK for development, but GNU ELPA should encourage
backward and forward compatibility.

>> There are lots of points where directory deletion happens in GNUmakefile
>> and archive-contents.el, during both "make externals" and "make
>> archive".
>
> Deletion in "make archive" is not a problem, AFAICT.
>
>> As for "make externals", I don't really like the concept of it, because
>> it mixes the build process with source configuration management.  For
>> example, this directory deletion could remove in-progress work from my
>> source directory, under elpa/packages:
>
> Exactly.
>
>>    ;; Check if `dir' is under version control.
>>    ((not (zerop (call-process "git" nil nil nil
>>                               "ls-files" "--error-unmatch" dir)))
>>     (message "Deleted untracked package %s" dir)
>>     (delete-directory dir 'recursive t))
>>
>> I would never expect that when invoking make.  I would rather "git
>> status" tell me that I have untracked changes.
>
> Yes, this is the problematic case.  This case is the one that intends to
> handle the situation where we removed a :core package from
> externals-list.
>
> Presumably the call-process check should make sure this directory is not
> under Git control at all, so no amount of "git status" will help.
>
> So here, instead of delete-directory we should use a new function which
> is a lot more careful, so it only deletes data which it has earlier
> created for a :core package.  If the :core packages are handled via
> symlinks, then this can be done by checking that the directory only
> contains symlinks (or subdirs with the same property).  Of course there
> are other ways, such as keeping track somewhere of the files we've
> installed as :core, and then check this info before we delete files.

OK, I'll work on this.

>> I didn't closely follow the discussions while this was being designed.
>> Was there some reason to avoid git submodules?
>
> "git submodules" could make sense (and I did consider them) for
> the :external packages, but these aren't affected by this code (since
> the call-process should indicate that they are under Git).
>
>> It seems possible to replace "make externals" with submodules that
>> track "remote" externals/ branches of elpa.git itself, with git >=
>> 1.8.2.
>
> IIRC the issue with git submodules is that they have to refer to
> a particular revision rather than to a branch, so additionally to
> committing new code on the external branch, you'd then have to go and
> commit a change to master that updates the submodule reference to point
> to the new revision at the head of the external branch.

That was true at one point but it looks like git 1.8.2 introduced the
notion of "remote-tracking branch submodules".  Maybe I'll experiment
with how that could work for elpa.git.

Thomas