Search Commits via Commit Hash

[gary-kim]

Closes #7395
Commit history can now be searched with commit hash.

EDIT: whoops, slight mistake in the test case. I'll fix it in a bit.
EDIT2: should be fixed now.

[codecov-io]

Codecov Report

Merging #7400 into master will increase coverage by 0.01%.
The diff coverage is 92.85%.

@@            Coverage Diff            @@
##           master   #7400      +/-   ##
=========================================
+ Coverage   41.59%   41.6%   +0.01%     
=========================================
  Files         480     480              
  Lines       64144   64160      +16     
=========================================
+ Hits        26681   26695      +14     
- Misses      34004   34007       +3     
+ Partials     3459    3458       -1

Impacted Files	Coverage Δ
modules/git/repo_commit.go	54% <92.85%> (+2.71%)	⬆️
models/unit.go	62.16% <0%> (-5.41%)	⬇️
models/repo_list.go	73.17% <0%> (-0.98%)	⬇️
modules/log/event.go	65.64% <0%> (+1.02%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6097ff6...1ff3ae4. Read the comment docs.

[lunny]

Maybe we could add an extra git log <keyword> -1 to get all output when keyword didn't containers : and then merge the output with original.

[gary-kim]

Maybe we could add an extra git log <keyword> -1 to get all output when keyword didn't containers : and then merge the output with original.

Wouldn't that only match a single commit hash? In Gitea's commit history, there are 4 hashes that start with 46d3 so doing git log 46d3 -1 only outputs the most recent one or errors, I don't remember which. Also, do we want this matching only from the start or from the middle of the commit hash as well?

EDIT: Okay, not 46d3 but I do remember finding a two commit hashes that had the same first 4 characters.

[lunny]

@gary-kim I think we should only match prefix what is the git log does.

[gary-kim]

@gary-kim I think we should only match prefix what is the git log does.

@lunny Okay, done.

[lunny]

@gary-kim Good work! Two small things we may need to consider:

• If a commit has another commit's id on commit message, we may get duplicated commit output, how to fix that?
• I find repo. parsePrettyFormatLogToList will get all commit information again via git command, so why we use prettyLogFormat parameter? Just parse the full commit information may give us better performance.
  Of course this issue is out of this PR. I just found that there.

[gary-kim]

• If a commit has another commit's id on commit message, we may get duplicated commit output, how to fix that?

I just pushed a commit that should solve this issue.

• I find repo. parsePrettyFormatLogToList will get all commit information again via git command, so why we use prettyLogFormat parameter? Just parse the full commit information may give us better performance.
  Of course this issue is out of this PR. I just found that there.

I did find that strange as well. Could I open a PR after this to try and address that?

[lunny]

• If a commit has another commit's id on commit message, we may get duplicated commit output, how to fix that?

I just pushed a commit that should solve this issue.

• I find repo. parsePrettyFormatLogToList will get all commit information again via git command, so why we use prettyLogFormat parameter? Just parse the full commit information may give us better performance.
  Of course this issue is out of this PR. I just found that there.

I did find that strange as well. Could I open a PR after this to try and address that?

Yes, please.

[gary-kim]

I did find that strange as well. Could I open a PR after this to try and address that?

I just took a better look at how parsePrettyFormatLogToList works and I think I'll need some time to understand this codebase a little more before working on this. If someone else would like to work on this, great, please go ahead! If not, I'll get back to this later.

[typeless]

@gary-kim There seems to be a more straightforward approach.

1. Scan each keyword and determine if it is a hex number (https://golang.org/pkg/encoding/hex/#DecodeString) and its length is longer than 4 characters (less than 4 characters won't be accepted by git as valid commit id).
2. If it is a hex number, collect it into a slice (named hashes, for example).
3. Append the hashes slice to the end of the command.

[gary-kim]

@gary-kim There seems to be a more straightforward approach.

1. Scan each keyword and determine if it is a hex number (https://golang.org/pkg/encoding/hex/#DecodeString) and its length is longer than 4 characters (less than 4 characters won't be accepted by git as valid commit id).

2. If it is a hex number, collect it into a slice (named hashes, for example).

3. Append the hashes slice to the end of the command.

git log only uses the first commit hash in the command so I don't think appending hashes to the end of the command is going to do anything.
Also, there would need to be a check to make sure it is actually a commit first anyways because parsePrettyFormatLogToList returns an error if any of the given commits are not actual commits.

[typeless]

@gary-kim Yeah, that does not work as I expected :(
My rationale is that it would be nice if the seemingly duplicate code paths (cmd and args) can be unified into one.
It is probably not easy though.

By the way, what is the expected behavior of the hash keyword? Do they OR or AND with other conditions?

[gary-kim]

By the way, what is the expected behavior of the hash keyword? Do they OR or AND with other conditions?

They will act similarly to the keywords. The commit needs to match all the given conditions such as author: and at least one of the keywords/hashes. From a user's standpoint, it's as if the hash has become part of the commit message.

My rationale is that it would be nice if the seemingly duplicate code paths (cmd and args) can be unified into one.

That is possible. Although, personally, I'd rather keep it as it is. Essentially, the original command can also built using the args slice at the end. The slight issue is what the command needs to run starts to really diverge before the command can be run which will make the last little bit of the function kind of messy.
@lunny What do you think?

[gary-kim]

@typeless I got rid of the duplicate code paths. What do you think?

[typeless]

@gary-kim LGTM

[lunny]

CI failed.

[gary-kim]

CI failed.

Fixed.

[lunny]

@typeless you are not a maintainer yet but you have satisfied the condition, please send a PR to apply for that. Otherwise your approval will not be counted.

[guillep2k]

Check for simplification on '\n' handling.