Improve ObjectFormat interface

[lunny]

The 4 functions are duplicated, especially as interface methods. I think we just need to keep MustID the only one and remove other 3.

MustID(b []byte) ObjectID
MustIDFromString(s string) ObjectID
NewID(b []byte) (ObjectID, error)
NewIDFromString(s string) (ObjectID, error)

Introduced the new interfrace method ComputeHash which will replace the interface HasherInterface. Now we don't need to keep two interfaces.

Reintroduced git.NewIDFromString and git.MustIDFromString. The new function will detect the hash length to decide which objectformat of it. If it's 40, then it's SHA1. If it's 64, then it's SHA256. This will be right if the commitID is a full one. So the parameter should be always a full commit id.

@AdamMajer Please review.

dismiss

[wxiaoguang]

Not sure whether it is right to remove objectFormat := git.ObjectFormatFromName(repo.ObjectFormatName); objectFormat.MustIDFromString(...)

[lunny]

Not sure whether it is right to remove objectFormat := git.ObjectFormatFromName(repo.ObjectFormatName); objectFormat.MustIDFromString(...)

Reintroduced git.NewIDFromString and git.MustIDFromString. The new function will detect the hash length to decide which objectformat of it. If it's 40, then it's SHA1. If it's 64, then it's SHA256. This will be right if the commitID is a full one. So the parameter should be always a full commit id.

[wxiaoguang]

Not sure whether it is right to remove objectFormat := git.ObjectFormatFromName(repo.ObjectFormatName); objectFormat.MustIDFromString(...)

Reintroduced git.NewIDFromString and git.MustIDFromString. The new function will detect the hash length to decide which objectformat of it. If it's 40, then it's SHA1. If it's 64, then it's SHA256. This will be right if the commitID is a full one. So the parameter should be always a full commit id.

I do not have full understanding, the only question is it good to "guess" the object format by length. I can accept if it is fine to do so.

[AdamMajer]

My thought was to move "the other way" when it comes to refactoring. That is, to move away from string and toward ObjectID and ObjectFormat to store these references everywhere. Then the string to ObjectID helper functions would go away naturally. Is this something that would be beneficial?

The only problem here is that you are removing objectFormat and assuming that hashes will always be in correct format, there will be an issue when you pass a partial hash the length of SHA1 into a SHA256 repository. Any user input cannot be assumed to be full hash just by looking at length, for example. So if we take care that user input never hits these functions, there should be no issue.

[lunny]

My thought was to move "the other way" when it comes to refactoring. That is, to move away from string and toward ObjectID and ObjectFormat to store these references everywhere. Then the string to ObjectID helper functions would go away naturally. Is this something that would be beneficial?

The only problem here is that you are removing objectFormat and assuming that hashes will always be in correct format, there will be an issue when you pass a partial hash the length of SHA1 into a SHA256 repository. Any user input cannot be assumed to be full hash just by looking at length, for example. So if we take care that user input never hits these functions, there should be no issue.

That's what GetCommit to do which convert a partial hash to a full one. And if it's a partial hash, the previous implemention of NewIDFromString still cannot resolve the problem. So we should not expect the new implemention can resolve the problem.