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

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

[Brent Goodrick]

The `sort' Elisp function works destructively, causing anomalies where
operations on multiple files would be performed on one file.  This
checkin works around that by doing a deep copy with `append'.

Also, git-add-file needed to pass 'modified to git-marked-files-state,
as otherwise, files that are modified but not yet in the index would
not show up in the git-marked-files-state return value, which would
then cause a prompt for file to show up when the files are clearly
marked in the status buffer.

Signed-off-by: Brent Goodrick <bgoodr@gmail.com>
---
 contrib/emacs/git.el |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/contrib/emacs/git.el b/contrib/emacs/git.el
index fcbe2d9..93e47c1 100644
--- a/contrib/emacs/git.el
+++ b/contrib/emacs/git.el
@@ -532,7 +532,7 @@ Each entry is a cons of (SHORT-NAME . FULL-NAME)."
 (defun git-status-filenames-map (status func files &rest args)
   "Apply FUNC to the status files names in the FILES list."
   (when files
-    (setq files (sort files #'string-lessp))
+    (setq files (sort (append files nil) #'string-lessp))
     (let ((file (pop files))
           (node (ewoc-nth status 0)))
       (while (and file node)
@@ -773,7 +773,7 @@ Return the list of files that haven't been handled."
   "Update the status of FILES from the index."
   (unless git-status (error "Not in git-status buffer."))
   ;; set the needs-update flag on existing files
-  (if (setq files (sort files #'string-lessp))
+  (if (setq files (sort (append files nil) #'string-lessp))
       (git-status-filenames-map
        git-status (lambda (info) (setf (git-fileinfo->needs-update info) t)) files)
     (ewoc-map (lambda (info) (setf (git-fileinfo->needs-update info) t) nil) git-status)
@@ -1041,7 +1041,7 @@ Return the list of files that haven't been handled."
 (defun git-add-file ()
   "Add marked file(s) to the index cache."
   (interactive)
-  (let ((files (git-get-filenames (git-marked-files-state 'unknown 'ignored))))
+  (let ((files (git-get-filenames (git-marked-files-state 'modified 'unknown 'ignored))))
     ;; FIXME: add support for directories
     (unless files
       (push (file-relative-name (read-file-name "File to add: " nil nil t)) files))
-- 
1.6.2.rc0.10.gf6b9.dirty

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

[Alexandre Julliard]

Brent Goodrick <bgoodr@gmail.com> writes:

> The `sort' Elisp function works destructively, causing anomalies where
> operations on multiple files would be performed on one file.  This
> checkin works around that by doing a deep copy with `append'.

This shouldn't be necessary, it's OK for git-status-update-files to
destroy the list. If there are callers that want the list to be
preserved they should save it themselves.

> Also, git-add-file needed to pass 'modified to git-marked-files-state,
> as otherwise, files that are modified but not yet in the index would
> not show up in the git-marked-files-state return value, which would
> then cause a prompt for file to show up when the files are clearly
> marked in the status buffer.

Not sure what you mean here, it should not be possible for a file to be
in modified state but not in the index. 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.

-- 
Alexandre Julliard
julliard@winehq.org

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

[Brent Goodrick]

Hi Alexandre (Warning: Incoming long-in-the-tooth email),

See my comments below:

 > From: Alexandre Julliard <julliard@winehq.org>
 > 
 > Brent> The `sort' Elisp function works destructively, causing anomalies where
 > Brent> operations on multiple files would be performed on one file.  This
 > Brent> checkin works around that by doing a deep copy with `append'.
 > 
 Alexandre> This shouldn't be necessary, it's OK for git-status-update-files to
 Alexandre> destroy the list. If there are callers that want the list to be
 Alexandre> preserved they should save it themselves.

Let me demonstrate the issue with the destructive sort showing that it
is a problem (but only part of the problem). This long sequence shows
my reasoning as to what led me to make this patch:

1. I created a scratch throwaway branch called
   bg/scratch-elisp-testing off of master in my git repo. In Emacs I
   executed M-x git-statux of my work area showing a couple of files
   that I do not want to git-add which I will leave alone, but
   otherwise there are no edits applied yet on the
   bg/scratch-elisp-testing throw away branch:

,----
| git status
| # On branch bg/scratch-elisp-testing
| # Untracked files:
| #   (use "git add <file>..." to include in what will be committed)
| #
| #	Documentation/share/
| #	patch
| nothing added to commit but untracked files present (use "git add" to track)
`----

2. I make editing changes to git.c and progress.c, but will refrain
   from executing git-add on those edits at this point.  Then I
   execute M-x git-status from within Emacs and see:

,----
| Directory:  ~/git_from_source/git/
| Branch:     bg/scratch-elisp-testing
| Head:       f6b98e46bd - git-web--browse: Fix check for /bin/start
| 
|      Unknown      Documentation/share/
|      Modified     git.c
|      Unknown      patch
|      Modified     progress.c
| 
`----

This corresponds to git status output that looks like this:

,----
| git status
| # On branch bg/scratch-elisp-testing
| # Changed but not updated:
| #   (use "git add <file>..." to update what will be committed)
| #   (use "git checkout -- <file>..." to discard changes in working directory)
| #
| #	modified:   git.c
| #	modified:   progress.c
| #
| # Untracked files:
| #   (use "git add <file>..." to include in what will be committed)
| #
| #	Documentation/share/
| #	patch
| no changes added to commit (use "git add" and/or "git commit -a")
`----

3. I would like those changes to be a part of the commit that I make,
   so I position the Emacs point onto "git.c" and type "m" to mark
   git.c, and do the same for progress.c. At this point, the
   *git-status* buffer shows as:

,----
| Directory:  ~/git_from_source/git/
| Branch:     bg/scratch-elisp-testing
| Head:       f6b98e46bd - git-web--browse: Fix check for /bin/start
| 
|      Unknown      Documentation/share/
|    * Modified     git.c
|      Unknown      patch
|    * Modified     progress.c
| 
`----

4. I now type "a" to add the marked files (my expectation is that
   git-add will be executed on "git.c" and "progress.c" but not the
   other two files which I did not mark). But instead I see in the
   minibuffer a prompt asking about which files to apply the git-add:

,----
| File to add: ~/git_from_source/git/
`----

5. I debugged a bit and thought all I needed to do was add the
   'modified symbol to the call to git-marked-files-state as this diff
   shows:

--- cut here ---
diff --git a/contrib/emacs/git.el b/contrib/emacs/git.el
index fcbe2d9..b8c268b 100644
--- a/contrib/emacs/git.el
+++ b/contrib/emacs/git.el
@@ -1041,7 +1041,7 @@ Return the list of files that haven't been handled."
 (defun git-add-file ()
   "Add marked file(s) to the index cache."
   (interactive)
-  (let ((files (git-get-filenames (git-marked-files-state 'unknown 'ignored))))
+  (let ((files (git-get-filenames (git-marked-files-state 'modified 'unknown 'ignored))))
     ;; FIXME: add support for directories
     (unless files
       (push (file-relative-name (read-file-name "File to add: " nil nil t)) files))
--- cut here ---

6. I ran "git reset git.c progress.c" to reset the state in git. So
   now git status reports similar to what it was when I started (but
   git.el is modified to add the 'modified symbol to the call to
   git-marked-fiels-state above):

,----
| git status
| # On branch bg/scratch-elisp-testing
| # Changed but not updated:
| #   (use "git add <file>..." to update what will be committed)
| #   (use "git checkout -- <file>..." to discard changes in working directory)
| #
| #	modified:   contrib/emacs/git.el
| #	modified:   git.c
| #	modified:   progress.c
| #
| # Untracked files:
| #   (use "git add <file>..." to include in what will be committed)
| #
| #	Documentation/share/
| #	patch
| no changes added to commit (use "git add" and/or "git commit -a")
`----

7. Kill the *git-status* buffer to get a clean slate (hopefully, that
   is all one has to do to reset git.el's state).

8. I ran M-x git-status again and saw:

,----
| Directory:  ~/git_from_source/git/
| Branch:     bg/scratch-elisp-testing
| Head:       f6b98e46bd - git-web--browse: Fix check for /bin/start
| 
|      Unknown      Documentation/share/
|      Modified     contrib/emacs/git.el
|      Modified     git.c
|      Unknown      patch
|      Modified     progress.c
| 
`----

9. I marked git.c and progress.c using "m" again and saw:

,----
| Directory:  ~/git_from_source/git/
| Branch:     bg/scratch-elisp-testing
| Head:       f6b98e46bd - git-web--browse: Fix check for /bin/start
| 
|      Unknown      Documentation/share/
|      Modified     contrib/emacs/git.el
|    * Modified     git.c
|      Unknown      patch
|    * Modified     progress.c
| 
`----

10. I then saw only one message output:

,----
| Added progress.c
`----

I should see two messages here, one for git.c and another for
progress.c. This is the reason for the addition of the append function
calls to copy the files list before sort sees it, as that is how they
are reported. But that is not the whole story, as I show below.

11. Looking at the *git-status* buffer, I do not see any status change
    for the files I have just added:

,----
| Directory:  ~/git_from_source/git/
| Branch:     bg/scratch-elisp-testing
| Head:       f6b98e46bd - git-web--browse: Fix check for /bin/start
| 
|      Unknown      Documentation/share/
|      Modified     contrib/emacs/git.el
|    * Modified     git.c
|      Unknown      patch
|    * Modified     progress.c
| 
`----

That seems confusing (but see my response to your comment below about
the three conceptual states that git has that the other SCM's I've
used (CVS, Perforce) don't seem to have).

12. I ran git status outside of Emacs and saw that the files were
indeed added:

,----
| # On branch bg/scratch-elisp-testing
| # Changes to be committed:
| #   (use "git reset HEAD <file>..." to unstage)
| #
| #	modified:   git.c
| #	modified:   progress.c
| #
| # Changed but not updated:
| #   (use "git add <file>..." to update what will be committed)
| #   (use "git checkout -- <file>..." to discard changes in working directory)
| #
| #	modified:   contrib/emacs/git.el
| #
| # Untracked files:
| #   (use "git add <file>..." to include in what will be committed)
| #
| #	Documentation/share/
| #	patch
`----

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.

 > 
 > > Also, git-add-file needed to pass 'modified to git-marked-files-state,
 > > as otherwise, files that are modified but not yet in the index would
 > > not show up in the git-marked-files-state return value, which would
 > > then cause a prompt for file to show up when the files are clearly
 > > marked in the status buffer.
 > 
 > Not sure what you mean here, it should not be possible for a file to be
 > in modified state but not in the index.

Yep. Shouldn't be but I think I have demonstrated that was the case
above. Let me know what I missed in my reasoning.

 > 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. 

In my view, there are three conceptual states that a file in git can
have:

 - The file is modified in the working tree but not yet added to the
   index (Locally-modified in my sample view below),

 - The file is modified, but also added to the index for the next
   commit and does not have any subsequent edits in the working tree
   (Ready-for-commit in my sample view below),

 - The file is added to the index previously, but the user made
   additional changes to the file that are not yet in the index, which
   means it potentially needs another add (Modified-but-has-edits in
   my sample view below).

I need have the *git-status* buffer to visually reflect those three
conceptual states. Otherwise there isn't much point to having M-x
git-status when I have to constantly run "git status" from the shell
to double-check what the *git-status* buffer should have been telling
me all along.

,----
| Directory:  ~/git_from_source/git/
| Branch:     bg/scratch-elisp-testing
| Head:       f6b98e46bd - git-web--browse: Fix check for /bin/start
| 
|      Unknown                       Documentation/share/
|      Locally-modified              contrib/emacs/git.el
|    * Modified-but-has-edits        git.c
|      Unknown                       patch
|    * Ready-for-commit              progress.c
| 
`----

I'm not insisting that the names "Locally-modified",
"Modified-but-has-edits", and "Ready-for-commit" be the ones actually
used (perhaps you can come up with names that better befit the git
environment), but something like the above would be better than just
having them all show up as "modified" for all three states.

Thanks,
Brent

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

[Alexandre Julliard]

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.

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.

-- 
Alexandre Julliard
julliard@winehq.org

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

[Brent Goodrick]

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

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

[Alexandre Julliard]

Brent Goodrick <bgoodr@gmail.com> writes:

> 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?

There's no manual, and I'm not going to write one, I suck at writing
documentation. If you would like to contribute one it would certainly be
welcome.

> 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?

That's exactly what git-success-message already does. The only problem
is that the list isn't always preserved properly (and that's only a
cosmetic bug, the operations get carried out correctly).

-- 
Alexandre Julliard
julliard@winehq.org

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

[Brent Goodrick]

> There's no manual, and I'm not going to write one, I suck at writing
> documentation. If you would like to contribute one it would certainly be
> welcome.

It's apparent that you are quite articulate in your emails, so I don't
buy the argument that you suck at writing documentation. ;)

Actually, there isn't enough functionality in git.el to write up in a
manual yet. Once there is, then perhaps having a manual might make
sense.

Let's just leave it as adding additional commentary at the top of
git.el that warns future users of the intent behind the user interface
so that you don't have to keep answering this question later on.  To
that end, here is a patch of my attempt at adding an explanatory
comment at the top of git.el (in lieu of a full-blown manual):

diff --git a/contrib/emacs/git.el b/contrib/emacs/git.el
index fcbe2d9..b383f51 100644
--- a/contrib/emacs/git.el
+++ b/contrib/emacs/git.el
@@ -23,8 +23,33 @@

 ;; This file contains an interface for the git version control
 ;; system. It provides easy access to the most frequently used git
-;; commands. The user interface is as far as possible identical to
-;; that of the PCL-CVS mode.
+;; commands. The user interface intensionally hides some of the full
+;; interface provided by the git command. This was done in order to
+;; keep the user interface similar to other Emacs SCM interface
+;; modes, such as the PCL-CVS mode. For example, it is possible using
+;; git directly from the command-line to do partial checkins such as:
+;;
+;;  - Make a first edit to an git-controlled file
+;;
+;;  - Add the file to the git index with the "git add" command.
+;;
+;;  - Make a second edit to that same file
+;;
+;;  - Checkin the change using the "git commit" command, which
+;;    results in the first edit being committed, but not the second.
+;;
+;; The Emacs Git interface as provided by the git-status command
+;; herein simplifies the checkin sequence by hiding the git index
+;; from the user. Instead, the buffer git-status shows will only show
+;; that the file has been modified, even if some of the modifications
+;; are not yet added to the git index. Only files not yet known to
+;; git have to be added using the "a" keybinding one time (i.e., you
+;; don't have to remember to re-add the second change as required by
+;; git in the example above). To commit, you mark multiple files in
+;; the git-status buffer using the "m" keybinding, and then commit
+;; those files with the "c" binding. Then git-status will insure that
+;; all edits made to those marked files are silently added to the git index
+;; before git commit is executed.
 ;;
 ;; To install: put this file on the load-path and place the following
 ;; in your .emacs file:


>
>> 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?
>
> That's exactly what git-success-message already does. The only problem
> is that the list isn't always preserved properly (and that's only a
> cosmetic bug, the operations get carried out correctly).

Agreed.  Thanks for your help!

bg