Re: Should we have a commit size guideline?

[David Kastrup]

Artur Malabarba <address@hidden> writes:

> 2015-12-15 12:49 GMT+00:00 David Kastrup <address@hidden>:
>> [Regarding commit 2e84888]
>>
>> This is a very, very large commit.  It should have been split into
>> multiple commits addressing separate issues.
>
> When commiting changes, I usually group them into the smallest
> possible commits while still leaving everything in a consistent state
> (i.e., not defining a function that's only used in later commits, not
> changing a function without making the necessary changes in other
> places that call this function). I find that this helps with both
> git-bisect and git-revert.

Sure.

But that commit is not in that class.

-(defconst dir-locals-file ".dir-locals.el"
+(defconst dir-locals-file ".dir-locals*.el"

is a change in default that is independent from the implementation and
that may or may not be the main cause of the performance regression, or
be responsible for some part of it.  As long as it is mixed in with the
rest, it's not easy to find out.

-       (message ".dir-locals error: %s" (error-message-string err))
+       (message "%s error: %s" dir-locals-file (error-message-string err))

is a bug fix that is independent from the implementation.

       (when (and (string-prefix-p (car elt) file
-                                 (memq system-type
-                                       '(windows-nt cygwin ms-dos)))
-                (> (length (car elt)) (length (car dir-elt))))
-       (setq dir-elt elt)))
+                                  (memq system-type
+                                        '(windows-nt cygwin ms-dos)))
+                 (> (length (car elt)) (length (car dir-elt))))
+        (setq dir-elt elt)))
     (if (and dir-elt

is a gratuitous spacing change.  So is most of

     (if (and dir-elt
-            (or (null locals-file)
-                (<= (length (file-name-directory locals-file))
-                    (length (car dir-elt)))))
-       ;; Found a potential cache entry.  Check validity.
-       ;; A cache entry with no MTIME is assumed to always be valid
-       ;; (ie, set directly, not from a dir-locals file).
-       ;; Note, we don't bother to check that there is a matching class
-       ;; element in dir-locals-class-alist, since that's done by
-       ;; dir-locals-set-directory-class.
-       (if (or (null (nth 2 dir-elt))
-               (let ((cached-file (expand-file-name dir-locals-file-name
-                                                    (car dir-elt))))
-                 (and (file-readable-p cached-file)
-                      (equal (nth 2 dir-elt)
-                             (nth 5 (file-attributes cached-file))))))
-           ;; This cache entry is OK.
-           dir-elt
-         ;; This cache entry is invalid; clear it.
-         (setq dir-locals-directory-cache
-               (delq dir-elt dir-locals-directory-cache))
-         ;; Return the first existing dir-locals file.  Might be the same
-         ;; as dir-elt's, might not (eg latter might have been deleted).
-         locals-file)
+             (or (null locals-dir)
+                 (<= (length locals-dir)
+                     (length (car dir-elt)))))
+        ;; Found a potential cache entry.  Check validity.
+        ;; A cache entry with no MTIME is assumed to always be valid
+        ;; (ie, set directly, not from a dir-locals file).
+        ;; Note, we don't bother to check that there is a matching class
+        ;; element in dir-locals-class-alist, since that's done by
+        ;; dir-locals-set-directory-class.
+        (if (or (null (nth 2 dir-elt))
+                (let ((cached-files (dir-locals--all-files (car dir-elt))))
+                  ;; The entry MTIME should match the most recent
+                  ;; MTIME among matching files.
+                  (and cached-files
+                       (= (time-to-seconds (nth 2 dir-elt))
+                          (apply #'max (mapcar (lambda (f) (time-to-seconds 
(nth 5 (file-attributes f))))
+                                               cached-files))))))
+            ;; This cache entry is OK.
+            dir-elt
+          ;; This cache entry is invalid; clear it.
+          (setq dir-locals-directory-cache
+                (delq dir-elt dir-locals-directory-cache))
+          ;; Return the first existing dir-locals file.  Might be the same
+          ;; as dir-elt's, might not (eg latter might have been deleted).
+          locals-file)
       ;; No cache entry.

That makes it harder to find the wheat in the chaff.  Basically, one has
to use git log -w -p or equivalent in order not to get distracted in
unrelated content.  And even then stuff is a bit arduous.

-- 
David Kastrup