Re: [PATCH] Fix file mark handling and sort side-effects in git.el

[Brent Goodrick <bgoodr@gmail.com>]

On Sun, Feb 15, 2009 at 9:08 AM, Alexandre Julliard <julliard@winehq.org> wrote:
>
> Brent Goodrick <bgoodr@gmail.com> writes:
>
> > My rationale for adding append function calls to the sort calls is to
> > leave the callers value alone since the caller needs to make use of
> > the list value in subsequent operations, especially for issuing
> > messages.
>
> My point is that the callers that need it should take care of it
> themselves, instead of forcing a copy even in cases where it's not
> necessary. And the copy can most likely be avoided completely by
> changing how the success message is printed.
>
> >  > If you mean using git-add-file to do an update-index on an already
> >  > tracked file, that's not what it's meant to do.
> >
> > That would be fine in, say, Perforce where once a file is added it
> > stays added even if the user mades additional edits. I don't agree
> > that is the best approach in the case the Emacs interface to git in
> > git.el, since there is that "third" state where I could have added the
> > file, then edited it, then forgot that I had edited it and proceeded
> > naively to commit, only to be surprised later that the subsequent edit
> > to the file was not committed.
>
> The design of git.el is that the index is not exposed directly, it's
> treated as an implementation detail. So "add" in git.el is only for
> adding an untracked file, it's not for updating the index contents of an
> already tracked file; that's an unnecessary operation since git.el uses
> the file marks to determine what gets committed.

Ok, now that makes sense to me. Part of the problem here is that there
is no statement in the user manual about git.el's intent to hide the
index. Perhaps something to the effect of "If you are new to using
Emacs but not new to git, then you need to know that bla bla ...".
Otherwise, I think users may get tripped up by this as I was. Was
there a manual in the works for git.el or did I just miss it in recent
checkins?

>
> It does get a bit confusing if you constantly mix command-line and
> git.el commands, but you are not supposed to do that, you should be able
> to do everything from the git.el buffer. I'm sure hiding the index
> offends the git purists, but IMHO it makes things more Emacs-ish and
> easier to use, especially if you are used to things like dired or
> pcl-cvs or vc-dir with other VC systems.

That makes sense to me from the Emacs standpoint.

However, there is still a minor bug: If you edit two new files (not
known to git), then run M-x git-status, those two new files will show
up as Unknown as expected. But, mark both of them, and type "a" will
show a message in the minibuffer for only one of the files. If you
then look in the *Messages* buffer, you will find that a message for
only one of the files.

However, the *git-status* buffer does properly reflect the two added
files by their state being changed to "Added". Since you may have a
ton of files that are being added, it probably doesn't make a whole
lot of sense to dump a long message into the minibuffer with all of
those names.  By the same token, it doesn't make sense to emit one
message per file either. Instead, would you be willing to change that
message to just state "Added n files" where "n" is the number of files
added?  I would provide a patch, but that patch would necessitate the
fix to the "files" variable being damaged by the sort function, since
the git.el code seems to not even to take that into account (and is
the root cause of why only one message is emitted AFAIK).  The other
alternative is to take the length of the files list before calling the
list-damaging functions, but that just seems so not in the spirit of
current Elisp coding practice.

Thanks,
bg