Added an option to force using commit hash + other stuff

[kaushalmodi]

• By default, the branch name is used in the copied link if available
• The defvar git-link-force-commit-hash-in-link when set to t forces the use of the commit hash in the copied link (even if a branch name like master is available.
• The interactive function git-link-force-hash set this var to true in a let before calling git-link.
• This commit does NOT change the default functionality of the package.

[kaushalmodi]

Solution for Issue https://github.com/sshaw/git-link/issues/12

[sshaw]

Hi, thanks for this. To clarify: If point is not on a commit hash, you want it to default to generating a link to the commit for the given line/region instead of the current branch? If so I like that idea, and find myself doing the same thing for the same reason when creating links in GitHub issues (though there I mostly use tags).

It may be better to make this part of git-link and enable it via a prefix arg or a defvar. No need for a separate function. Ultimately I'd like to have one core function: git-link-dwim.

The variable name git-link-force-commit-hash-in-link is a bit verbose. Maybe git-link-use-commit is better? Thoughts?

I do need to sort out #8, #9, and #11 first.

[kaushalmodi]

If point is not on a commit hash, you want it to default to generating a link to the commit for the given line/region instead of the current branch?

Yes

It may be better to make this part of git-link and enable it via a prefix arg or a defvar. No need for a separate function.

You were already using prefix arg for prompt. So I did not want to disturb that. You might be open to supporting multiple values of the prefix arg: C-u -> 4, C-u C-u -> 16, ...

Ultimately I'd like to have one core function: git-link-dwim.

That would be great.

The variable name git-link-force-commit-hash-in-link is a bit verbose. Maybe git-link-use-commit is better? Thoughts?

Agreed.

I do need to sort out #8, #9, and #11 first.

Once those requests are sorted out, I can continue on improving this pull request.

Thanks!

[sshaw]

Also, pull in latest master and squash future commit. Thanks.

[kaushalmodi]

Thanks. I will take out some time by next week. I have not used git for anything much else than commits to my .emacs.d. So I'll need time to figure out how to do what you suggested.

Basically I need to rebase my fork to your latest version first and replace this pull request with the one with my changes on top of the rebased fork? But I need to figure out how to do that.

[sshaw]

Actually based on the latest observations I would just toss this and start anew based on the current master. It'll be easier.

[kaushalmodi]

Can you please review this? If this is clean, then we can find a solution on how not to have a separate git-link-force-hash defun.

[sshaw]

Hi @kaushalmodi, I missed some of the other commits earlier.

Can you just submit the git-link-use-commit stuff as now it's mixed with the deactivate-mark and line number fixes.

It's easy for me to tackle one issue at a time.

Thanks for all the help!

[kaushalmodi]

The changes don't overlap and not many lines have changed. Can you please review these this time?

Also, how do I control what gets appended to the pull request?

[kaushalmodi]

Cleaned up the commits a bit. But I still can't figure out how to gate the later commits.

[sshaw]

The changes don't overlap and not many lines have changed. Can you please review these this time?

Yes, I've pulled in e560b41.

Also, how do I control what gets appended to the pull request?

Anything on the branch for the pull request will be added. If there's something unrelated to the request just create a new branch and commit the work there. In general it's good to stick to one feature or bug fix per request.

As for 717f46f this is caused by your emacs config. But, this is something that should be resolved. I'm just not sure if 717f46f is the right way. I'm open to other suggestions but will have to do some research myself.

I'll also have a look at aee3689.

Thanks again!

[kaushalmodi]

Yes, I've pulled in e560b41.

Thanks!

If there's something unrelated to the request just create a new branch and commit the work there. In general it's good to stick to one feature or bug fix per request.

Thanks, will do so now on.

As for 717f46f this is caused by your emacs config.

Wow! Thanks for culling that from my config.

I'm just not sure if 717f46f is the right way. I'm open to other suggestions but will have to do some research myself.

Once you are done copying the git-link, the region selection shouldn't be needed. Is that a correct assumption? If so, deactivating mark would be harmless. I have needed to do the same in a different function too. Well, the source from which that function was derived already had that deactivate-mark line.

I'll also have a look at aee3689.

Thanks!

[sshaw]

Once you are done copying the git-link, the region selection shouldn't be needed. Is that a correct assumption?

I think so.

If so, deactivating mark would be harmless.

After some research I think it's better to go with (setq deactivate-mark t) over (deactivate-mark).

• (deactivate-mark) does a bunch of other things, from C-h f deactivate-mark:

Deactivate the mark.
If Transient Mark mode is disabled, this function normally does
nothing; but if FORCE is non-nil, it deactivates the mark anyway.

Deactivating the mark sets `mark-active' to nil, updates the
primary selection according to `select-active-regions', and runs
`deactivate-mark-hook'.

If Transient Mark mode was temporarily enabled, reset the value
of the variable `transient-mark-mode'; if this causes Transient
Mark mode to be disabled, don't change `mark-active' to nil or
run `deactivate-mark-hook'.

My conclusion here: side effects are bad, especially if you don't know how they will manifest themselves. Of course my reasoning here could be seen as a side effect of my naïveté.

• kill-region uses (setq deactivate-mark t)

What do you think?

I have needed to do the same in a different function too. Well, the source from which that function was derived already had that deactivate-mark line.

That is a very nice function.

[kaushalmodi]

Here's a rough survey of how many times the function deactivate-mark is used vs just the setq.

It looks like its use as function is more prominent. But using simply (setq deactivate-mark t) worked for me too. So I will update the pull request with that.

But now I am curious to know when one should be used vs the other.

Used as a function:

km²~/downloads/:git/emacs> ag '\(deactivate-mark[\st]*\)' -G '\.el' -c
emacs-lisp/ert-x.el:1
emulation/cua-base.el:1
emulation/edt.el:1
emulation/viper-util.el:1
erc/erc-goodies.el:2
emacs-lisp/tcover-ses.el:1
gnus/nnnil.el:1
isearch.el:1
info.el:2
international/mule.el:2
leim/quail/hangul.el:1
mail/rmail.el:1
mwheel.el:1
replace.el:2
mouse.el:5
obsolete/mouse-sel.el:1
obsolete/tpu-edt.el:1
org/org-w3m.el:1
ses.el:1
ps-print.el:1
org/org.el:4
progmodes/f90.el:2
skeleton.el:1
tempo.el:1
term/ns-win.el:2
startup.el:1
term.el:1
textmodes/picture.el:2
simple.el:10
textmodes/reftex-index.el:1
textmodes/bibtex.el:1
type-break.el:1
vc/ediff-util.el:1
winner.el:1

Only setting the var:

km²~/downloads/:git/emacs> ag '\(setq deactivate-mark t\)' -G '\.el' -c
delsel.el:1
emulation/cua-rect.el:1
emulation/cua-base.el:1
indent.el:1
ses.el:2
rect.el:2
register.el:4
simple.el:2

[sshaw]

Interesting to see how the usage of the two are distributed. In the case of (deactivate-mark) why do they look to all be on line one? Maybe I'm not understanding the output?

[kaushalmodi]

The number following the colon is the number of occurrences in that file. It is because I use the -c argument for ag.

To learn more, I have posted this on emacs.SE.

[kaushalmodi]

Updated the pull req.

[kaushalmodi]

Thanks! I have squashed the bug-fix commit into the previous one and tested this one to work fine with and without region selections.

[kaushalmodi]

I first got the solution with this convoluted logic,

          (when (or
                 ;; when region is selected from top to bottom
                 (and (> (point) (mark))
                      (= end (line-beginning-position)))
                 ;; when region is selected from bottom to top
                 (and (< (point) (mark))
                      (= start (line-beginning-position))))
            (setq end (1- end)))

but then came up with a better solution; squashed and pushed the commit.

The point is not affected as stuff happens in save-excursion.

[kaushalmodi]

Thanks. This pull request was a great learning experience.

[sshaw]

Thanks.

So seems like there's no way to get the beginning of a line's position without moving point huh?

[kaushalmodi]

There is.. but by using the (when (or...)) logic in the above comment, which is a bit convoluted compared to the current solution. Also the point is unaffected in the current solution as we use save-excursion.