sshaw / git-link

Emacs package to get the GitHub/Bitbucket/GitLab/... URL for a buffer location
399 stars 74 forks source link

Allow subdomain git remotes #40

Closed kaushalmodi closed 7 years ago

kaushalmodi commented 7 years ago

UPDATE (2017/02/18)

The git-link and git-commit-link functions will now work even if the remote-host is not exactly "gitlab.com", but something like "gitlab.COMPANY.com" or "gitlab.COMPANY.org" or "gitlab.COMPANY.co.in".

This commits adds the use of cl-loop macro. If using emacs 24.x or newer, this is in-built in emacs in cl-macs library (which is required by the cl-lib library).

But for older emacs versions, the cl-lib library will be auto-installed from GNU Elpa with the help of the Package-Requires line added in this commit:

;; Package-Requires: ((cl-lib "0.6.1"))
sshaw commented 7 years ago

functions will work even if the remote-host is something like "gitlab.COMPANY.com" instead of...

Hi. Thanks but this is already possible and I don't think having what's perceived as being a catch-all makes sense as the subdomain can be named anything, right? Someone may call it gl.company.com or gh.company.com.

kaushalmodi commented 7 years ago

You're right. Will close this PR.

kaushalmodi commented 7 years ago

I still like the regexp approach more. What if that regexp is made a defcustom?

I would rather have the regexp "gitlab\\.\\(.+\\.\\)*com" in my emacs git repo than 2 add-to-list forms with company-specific domains in a separate non-public emacs config.

kaushalmodi commented 7 years ago

.. Or, instead of a regexp defcustom, the default regexp for Gitlab can be just "gitlab". I believe that will work for most people.

If that doesn't work, then they can just the regexp that works for them using the add-to-list method.

kaushalmodi commented 7 years ago

I have force-pushed an update to my origin branch. Please review again.


The git-link and git-commit-link functions will now work even if the remote-host is not exactly "gitlab.com", but something like "gitlab.COMPANY.com" or "gitlab.COMPANY.org" or "gitlab.COMPANY.co.in".

This commits adds the use of cl-loop macro. If using emacs 24.x or newer, this is in-built in emacs in cl-macs library (which is required by the cl-lib library).

But for older emacs versions, the cl-lib library will be auto-installed from GNU Elpa with the help of the Package-Requires line added in this commit:

;; Package-Requires: ((cl-lib "0.6.1"))
sshaw commented 7 years ago

.. Or, instead of a regexp defcustom, the default regexp for Gitlab can be just "gitlab". I believe that will work for most people.

I mean I do kinda like this idea, but it presents problems if say if my repository is at gitlab.com/sshaw/github-client. It will create a link to http://github.com/sshaw/github-client/blob/master/foo.whatever#L199

sshaw commented 7 years ago

Wait, actually no it wont. Almost forgot how this thing works 😄. Let me have an in-depth look.

kaushalmodi commented 7 years ago

Yeah, that regexp applies to only the remote base "github

kaushalmodi commented 7 years ago

Ah! On phone.. mis-clicked on Close.

Continuing.. remote base "github.com".

kaushalmodi commented 7 years ago

As a side, I just happened to notice the TODO: Consolidate `git-link--alist's*.

I was just thinking why we had 2 alists.. how about elements of type (REMOTE-LINK-REGEXP . (FN-LINK FN-COMMIT-LINK))?

If you are willing, I can work on that PR.

kaushalmodi commented 7 years ago

Made another forced push with README update.

sshaw commented 7 years ago

But for older emacs versions, the cl-lib library will be auto-installed... with the help of the Package-Requires line added in this commit: ;; Package-Requires: ((cl-lib "0.6.1"))

Why this version. I would think 0.5.

I was just thinking why we had 2 alists.. how about elements of type (REMOTE-LINK-REGEXP . (FN-LINK FN-COMMIT-LINK))?

Not sure on this, though offhand I don't know what the other options are.

What about git-link-homepage-XXXX, would that just get tacked on to the value list? (Now there's just 1 function for this, but it's possible this may need to be version specific.)

Regardless I'd keep both alists around with the new scheme (whatever that would be) for sometime to allow folks to migrate.

If you are willing, I can work on that PR.

I think adding tests are more important than alist consolidation 😸

kaushalmodi commented 7 years ago

0.6.1 is the latest cl-lib version available on GNU Elpa. Is there a reason to use version 0.5?

Learning to use ert is on my list.. some day. I am working on few packages, so will learn to write tests eventually.

sshaw commented 7 years ago

Is there a reason to use version 0.5?

Compatibility with versions <= 23.3 (I think) for free. Not sure of any downsides. It's what I've used elsewhere.

kaushalmodi commented 7 years ago

Compatibility with versions <= 23.3 (I think) for free. Not sure of any downsides. It's what I've used elsewhere.

The commentary in cl-lib 0.6.1 also says:

;; This is a forward compatibility package, which provides (a subset of) the ;; features of the cl-lib package introduced in Emacs-24.3, for use on ;; previous emacsen (it should work on Emacs≥21 as well as XEmacs).

I would say that it is safe to use 0.6.1.

The 0.6 and 0.6.1 updates happened very recently in Jan 2017. So that's why it's not yet out in the wild.

how about elements of type (REMOTE-LINK-REGEXP . (FN-LINK FN-COMMIT-LINK))?

Not sure on this, though offhand I don't know what the other options are.

Does it mean it's OK to go ahead with that? :)

What about git-link-homepage-XXXX, would that just get tacked on to the value list? (Now there's just 1 function for this, but it's possible this may need to be version specific.)

Yes, why not. We can put that same fn name in all lists for now.

Regardless I'd keep both alists around with the new scheme (whatever that would be) for sometime to allow folks to migrate.

That's understood.

kaushalmodi commented 7 years ago

Made another forced update on this PR branch.

Now using cl-find-if instead of cl-loop.

cl-seq will be autoloaded by cl-find-if. So doesn't need any explicit requires.

Here is my poor man test for this change:

(let ((hosts '("gitlab.com"
               "gitlab.example.com"
               nil
               "gh.com"
               "bitbucket.com"
               "github.com"
               "github.example.com"
               "bitbucket.com"
               "gh.example.com"
               "gitorious.com"))
      (alist '(("github" git-link-github)
               ("bitbucket" git-link-bitbucket)
               ("gitorious" git-link-gitorious)
               ("gitlab" git-link-gitlab)
               ("gh\\.example" git-link-gitlab)))
      (n 1)
      host)
  (dolist (host hosts)
    (message "host = %20S  returned fn = %S" host
             (when (stringp host)
               (nth n (cl-find-if (lambda (lst)
                                    (string-match-p (car lst) host))
                                  alist)))))
  nil)
sshaw commented 7 years ago

how about elements of type (REMOTE-LINK-REGEXP . (FN-LINK FN-COMMIT-LINK))?

Not sure on this, though offhand I don't know what the other options are.

Does it mean it's OK to go ahead with that? :)

I'd say hold-off for now. Need to think about it a bit more.

kaushalmodi commented 7 years ago

I pushed the changes as you commented on the commits yesterday. Let me know if you are waiting for something from me.

kaushalmodi commented 7 years ago

Please review; I have made couple of doc-string and compilation warning fixes.

kaushalmodi commented 7 years ago

Now compilation is squeaky clean even with lexical-binding enabled.

kaushalmodi commented 7 years ago

I have rebased this PR to your master and replaced nth 1 with cadr. Please review .

sshaw commented 7 years ago

Hi, looks like the rebase is messed up. Take a look please.

kaushalmodi commented 7 years ago

Sorry, how do I know that rebasing is wrong? I see this.. 3 commits rebased to the current upstream (sshaw/git-link) master.

image

sshaw commented 7 years ago

Looks like you've merged things into your branch, then rebased against master, then merged some more stuff into your branch 😕.

A few solutions come to mind:

  1. Drop the merge commits (not shown in above screenshots, but seen with git log) and then rebase.

  2. Create a patch file from a7123cdd4ae2c55cc59dd17c1c657ef6ca3c04f4, 79d6a590581e01137dbb6cc1e49e3b4d9fda1729, and b403c1c2e7fb408a00051c05b082986cb2227840 and just apply that to a new branch (though b403c1c and a7123cd should be squashed into a singe commit IMHO)

I think the 2nd may be better (untested):

git remote add sshaw SSHAW_GIT_URL
git log -1 -p a7123cdd4ae2c55cc59dd17c1c657ef6ca3c04f4 > /tmp/p1.patch
git log -1 -p b403c1c2e7fb408a00051c05b082986cb2227840 > /tmp/p1.patch
git log -1 -p 79d6a590581e01137dbb6cc1e49e3b4d9fda1729 >> /tmp/p2.patch
git checkout -b new-branch  sshaw/master
git apply -3 /tmp/p1.patch # 3-way merge in case of issues
git commit -a
git apply -3 /tmp/p2.patch
git commit -a 
sshaw commented 7 years ago

Hurrmmm actually options 2 seems like it may be more work.

kaushalmodi commented 7 years ago

Please check now. I finally understood my mistake.. I was doing merges instead of rebases. Magit made it a breeze to fix this.

sshaw commented 7 years ago

Great, thanks! 😅