Introduce magit-simple-log #609

Closed
wants to merge 2 commits into
from

4 participants

@artagnon

I've taken care not to stomp any variables so that magit-log can work side-by-side just as before.

Thanks.

@artagnon

Hm, there's a build failure. Have to investigate.

@sigma sigma and 1 other commented on an outdated diff Apr 2, 2013
contrib/magit-simple-keys.el
@@ -26,7 +26,7 @@
(require 'magit)
(defvar magit-key-mode-mapping
- '((logging magit-display-log)
+ '((logging magit-simple-log)
@sigma
Magit member
sigma added a line comment Apr 2, 2013

not sure this should be the default for simple keys. I mean, I think people are generally happier with the graph than without.

Also, this simple log should be made available to users of magit-key-mode.el

@artagnon
artagnon added a line comment Apr 2, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@artagnon

It should be good to merge now: I changed the name to magit-log-simple to match magit-log-long. I thought about the default for magit-simple-keys.el, and decided that to keep magit-log-simple there. My current rationale is that I'm the original author of magit-simple-keys.el and magit-log-simple, so it's likely that users of magit-simple-keys.el will find magit-log-simple more useful too. If it turns out that I'm wrong about this, we can always revert it to magit-log in the future.

Thanks.

@artagnon

@sigma Does this look good? Can we merge?

@tarsius
Magit member

The new command works the first time. When it or another log command is called a second time Emacs hangs and then (as I learned while typing this) finally crashes.

Also could you please move the simple log after the long log.

@artagnon

I was quite shocked to see this: my code obviously worked at the time of writing, and it's simple enough to not cause such catastrophic breakages. Turns out that there was a big regression that I rebased on top of: log-simple simply highlighted the underlying bug. After a very painful session of bisecting by hand, I finally found the regression: f0a9ca8 introduced by @yann. We can investigate what exactly went wrong later: for now, it is imperative to revert the commit.

I've pushed out an updated version of my branch fixing this regression and then introducing log-simple.

Thanks for looking into it.

@artagnon

Also, Travis builds are failing. Another regression?

artagnon added some commits May 14, 2013
@artagnon artagnon Revert "fix issue 628"
This reverts commit f0a9ca8.  It is the
cause for a very serious regression that makes Emacs hang and crash
randomly on log operations.  While the exact nature of the bug is still
unknown, revert this immediately.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
14b3f2d
@artagnon artagnon Introduce magit-log-simple for graph-less log
magit-log is only useful when navigating recent history.  To navigate
larger histories, it is imperative to turn off the graph and increase
the default number of entries displayed.  Introduce a magit-log-simple
that does exactly this.  Also teach magit-key-mode.el about it, and
switch contrib/magit-simple-keys.el to use it by default.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
cfe367f
@tarsius
Magit member

Travis is complaining a lot lately, so the error is probably not due to your changes. Just run the tests locally (you might have to edit test/run-tests.el to adjust the load-path).

As for reverting that commit I will wait for sigma to look into this but if he hasn't done anything by tonight I will take care of it. I would prefer not to revert the complete commit, as there really are two changes:

  1. make magit-log-author-date-overlay local which wasn't local at all before
  2. make some other variables permanently local

Do you know which of these changes made the difference? In either case it is probably one of these dynamic vs. local binding issues.

@sigma sigma was assigned May 14, 2013
@sigma
Magit member

Hmm somehow my comment was lost..
So, before we revert anything, we should understand what's happening exactly. Is #650 fixing that issue, independently of the state of this pull request ?

@tarsius
Magit member

Huh? And I am pretty sure I have commented on #650... github is eating our comments!

@tarsius
Magit member

magit-configure-have-graph is used to determine whether the installed version of git supports the --graph argument, modifying it like you do changes it's semantics and is wrong.

@artagnon

Huh? You're against it taking an optional argument to skip the check and set magit-have-graph to nil? How else would you write magit-log-simple?

@tarsius
Magit member

Yes. I would add an additional test instead of manipulating the current test to return what we need instead of what is correct. When we ask "does git-log support --graph", and that is in fact so, then we want the answer to be "yes" -- always).

@tarsius tarsius was assigned May 29, 2013
@tarsius
Magit member

I agree that such functionality would be desirable but I prefer the approach roughly outlined in the mentioned issue.

@tarsius tarsius closed this May 30, 2013
@afrepues

+1 for the #671 approach

Creating another action for what is actually a boolean option doesn't feel right (sorry for being so subjective), adds an odd behaviour to the way magit-log works (though there might be another odd duck there already...).

@tarsius
Magit member

Done using the #671 approach.

@tarsius tarsius added this to the 2.1.0 milestone Feb 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment