fix: fixed problem adding multiple repos with same name

[quintesse]

trafficstars

Fixes #1293

[codecov[bot]]

Codecov Report

Merging #1295 (1cf630c) into main (8a67177) will decrease coverage by 0.06%. The diff coverage is 45.45%.

@@             Coverage Diff              @@
##               main    #1295      +/-   ##
============================================
- Coverage     59.15%   59.09%   -0.07%     
- Complexity     1154     1155       +1     
============================================
  Files            97       97              
  Lines          6020     6028       +8     
  Branches        998     1000       +2     
============================================
+ Hits           3561     3562       +1     
- Misses         1951     1958       +7     
  Partials        508      508              

Flag	Coverage Δ
Linux	57.86% <45.45%> (-0.07%)	:arrow_down:
Windows	57.91% <45.45%> (-0.07%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ava/dev/jbang/dependencies/DependencyResolver.java	80.00% <ø> (ø)
...rc/main/java/dev/jbang/dependencies/MavenRepo.java	66.66% <40.00%> (-33.34%)	:arrow_down:
...in/java/dev/jbang/dependencies/DependencyUtil.java	81.08% <100.00%> (ø)

---

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 8a67177...1cf630c. Read the comment docs.

[maxandersen]

...i'm really not a fan of having the public api say Set when its not true.

Is it really that worse to take in as a List and deduplicate it before/after ?

[quintesse]

Not necessarily worse, no. You'd have to make sure that at any point where the list must be dedupped it actually is without duplicates. With LinkedHashSet you make sure it adheres to both requirements, ordered and unique. But I understand your misgivings.

[maxandersen]

thinking more about if you have same id with different url it should be an error.

if same id with same url we could potentially let it pass.

[quintesse]

Fail? Are you sure? Not simply a warning? There might be times that it might be hard to dedup those values in a script while it's easy enough for us to do in code...

[maxandersen]

A stern warning at least. Probably for any kind of duplication.

Which also implies we can't make it a set as then we loose the ability to know if duplicates occurs.

[quintesse]

I just now read more carefully what you wrote earlier: "same id with different url it should be an error"

So I understand now what you were trying to say and my first reaction was to agree with you, but on the other hand it's so easy to add additional repos that would completely override previous ones that I'm not sure that failing wen a duplicate is detected gives any advantages? What could go wrong if we let users do this?

[maxandersen]

Well you could resolve different artifacts than you expect and basically be able to enable a easy man in middle attack.

I still think it should be a hard error if something brings in diffferent url different id.

Actually thinking about it we probably should allow explicit control of repos ego prevent any kind of tampering.

[quintesse]

But aren't you deciding to run that code anyway and couldn't do that code much worse things than overriding a repo url? Anyway, I'm just discussing this for discussion's sake, I don't mind at all making this a hard error.

[maxandersen]

adding same id with same repo is fine, but trying add with same id but different repo should be error.

[maxandersen]

this is out of date/not relevant anymore - right @quintesse ?

[quintesse]

Out-of-date yes, but relevant... erm... I'm not 100% sure TBH. Perhaps we should check 😅