Should we have a commit size guideline? (was: builds are getting slower?)

Should we have a commit size guideline? (was: builds are getting slower?)

[Artur Malabarba]

2015-12-15 12:49 GMT+00:00 David Kastrup <dak@gnu.org>:
> [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.

If we have a different policy (maybe we should) I'm happy to adhere.


Cheers,
Artur

Re: Should we have a commit size guideline?

[David Kastrup]

Artur Malabarba <bruce.connor.am@gmail.com> writes:

> 2015-12-15 12:49 GMT+00:00 David Kastrup <dak@gnu.org>:
>> [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

Re: Should we have a commit size guideline? (was: builds are getting slower?)

[Eli Zaretskii]

> Date: Tue, 15 Dec 2015 13:48:28 +0000
> From: Artur Malabarba <bruce.connor.am@gmail.com>
> Cc: emacs-devel <emacs-devel@gnu.org>
> 
> > 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.
> 
> If we have a different policy (maybe we should) I'm happy to adhere.

I generally prefer to see the commits in one go, rather than split.
It makes it easier for me to review it.

Re: Should we have a commit size guideline?

[John Wiegley]

>>>>> Eli Zaretskii <eliz@gnu.org> writes:

> I generally prefer to see the commits in one go, rather than split. It makes
> it easier for me to review it.

The best of both worlds should be: A commit series from a feature branch,
merged into the target branch. This allows using "commit^..commit" to view all
the changes in one go against the target branch, while just "commit" will show
the series and it evolved on the feature branch.

Rebasing the feature branch also makes this nicer to read, as it omits
possibly frequent back-merges from the target into the feature branch as it
was being worked on.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: Should we have a commit size guideline?

[Paul Eggert]

John Wiegley wrote:
> The best of both worlds should be: A commit series from a feature branch,
> merged into the target branch.

I generally find it a waste of time to split a small or medium-sized patch into 
the maximum number of patchlets that can be applied one at a time without 
breaking the build. Of course it's not OK to amalgamate unrelated patches 
together into one huge commit. Still, for a relatively small and coherent patch 
such as the one that prompted this thread, we've already wasted more time 
discussing how to split it up than any possible benefit that would have accrued 
by splitting it.

That being said, it is annoying when part of a patch consists entirely of 
changing tabs to spaces or vice versa, as this one did, as this wastes the time 
of reviewers for no good reason. I wish we'd stop doing that. This particular 
botch was undoubtedly caused by the line "(emacs-lisp-mode . ((indent-tabs-mode 
. nil)))" in .dir-locals.el, which was recently inserted in the (vain) attempt 
to get rid of all the tabs in .el source files, and we should probably remove 
that line as in practice it really causes more trouble than it's worth.

Re: Should we have a commit size guideline?

[Dmitry Gutov]

On 12/16/2015 02:13 AM, Paul Eggert wrote:

> That being said, it is annoying when part of a patch consists entirely
> of changing tabs to spaces or vice versa, as this one did,

Did it? Emacs doesn't change tabs to spaces if the indentation level of 
the line hasn't changed.

And anyway, we've always considered tabs-to-spaces changes near some 
actual changed code to be okay. If someone is having a problem reading 
such diffs, Emacs has a wonderful diff-auto-refine-mode.

> This particular botch was undoubtedly caused by the line
> "(emacs-lisp-mode . ((indent-tabs-mode . nil)))" in .dir-locals.el,
> which was recently inserted in the (vain) attempt to get rid of all the
> tabs in .el source files, and we should probably remove that line as in
> practice it really causes more trouble than it's worth.

You're assuming that otherwise every committer would have 
indent-tabs-mode set to nil, which undoubtedly isn't true.

Even if it were true, were this line to be removed, you'd still see the 
same problem with people changing indentation from spaces to tabs in the 
same fashion. There are a lot of lines indented with spaces already.

Re: Should we have a commit size guideline?

[Paul Eggert]

Dmitry Gutov wrote:
> Emacs doesn't change tabs to spaces if the indentation level of the line
> hasn't changed.

I observe problems in this area when changing code (which changes indentation) 
and then changing it back (i.e., not undo, but make a further change that undoes 
most or all of the original change). In this case Emacs changes tabs to spaces, 
assuming the .dir-locals.el settings already mentioned.

> we've always considered tabs-to-spaces changes near some actual
> changed code to be okay.

Yes, of course.  Here, though, a two-line changed ballooned into a 23-line 
change. The first 2 lines of the ballooned change were real, and the other 21 
lines were tabs-to-spaces only.

It is a bit of an annoyance, that's all. Obviously there are workarounds. It's 
not something I'd bother writing about, but I'm sympathetic to those bothered 
enough to write.

indent-tabs-mode setting in Emacs's dir-locals.el

[Dmitry Gutov]

On 12/16/2015 03:00 AM, Paul Eggert wrote:

> I observe problems in this area when changing code (which changes
> indentation) and then changing it back (i.e., not undo, but make a
> further change that undoes most or all of the original change).

Indeed, that would do it.

> In this
> case Emacs changes tabs to spaces, assuming the .dir-locals.el settings
> already mentioned.

Yup. But it only makes a difference if Arthur hasn't customized 
indent-tabs-mode to nil in his Emacs already.

Having a project-wide setting, however, lets us hope that someday the 
indentation style will be uniform, and this problem won't occur anymore.

> Yes, of course.  Here, though, a two-line changed ballooned into a
> 23-line change. The first 2 lines of the ballooned change were real, and
> the other 21 lines were tabs-to-spaces only.

I usually try to undo such hunks with `diff-hl-revert-hunk'.

Re: Should we have a commit size guideline?

[Artur Malabarba]

[-- Attachment #1: Type: text/plain, Size: 1772 bytes --]

On 15 Dec 2015 2:23 pm, "David Kastrup" <dak@gnu.org> wrote:
>
> Sure.
>
> But that commit is not in that class.
>
> -(defconst dir-locals-file ".dir-locals.el"
> +(defconst dir-locals-file ".dir-locals*.el"

I considered this part of the same feature. But I agree it could have been
made on a follow up commit.

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

True!

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

Oops!

> So is most of
>
>      (if (and dir-elt
> -            (or (null locals-file)
> -                (<= (length (file-name-directory locals-file))
> -                    (length (car dir-elt)))))

Hm, I see what you mean. I generally trust emacs to not change
tabs-to-spaces in lines I didn't touch. Maybe the indentation there was
actually wrong (I'm reading this on a variable space font, so I can't
tell), or maybe I changed something that changed the indentation and then
changed it back again.

Anyway, I usually lookout for this before pushing, but in this case the
indentation changes are quite mixed with code changes so it probably
escaped me. Sorry about that.

[-- Attachment #2: Type: text/html, Size: 2406 bytes --]