On 10/05/21 at 06:53pm, Morgan Adamiec wrote: > > > On 05/10/2021 18:49, Morgan Adamiec wrote: > > > > > > On 05/10/2021 18:10, Andrew Gregory wrote: > >> On 10/05/21 at 12:53pm, Morgan Adamiec wrote: > >>> On 4 Oct 2021 8:28 pm, Andrew Gregory <[email protected]> > >>> wrote: > >>> > >>> > >>> On 10/04/21 at 08:09pm, morganamilo wrote: > >>> > >>> > This is the error value generally used and the calling function > >>> > >>> > explicitly checks for -1, later causing the error to be missed > >>> > >>> > and the transaction to continue. > >>> > >>> > >>> > >>> This result is not compared to -1, the result of download_files is. > >>> If > >>> we want > >>> > >>> to guarantee that download_files will return -1 on error, that's > >>> where > >>> the > >>> > >>> return should be normalized, not in find_dl_candidates. Tying the > >>> API > >>> of one > >>> > >>> function to another like this is just going to cause confusion and > >>> > >>> breakage > >>> > >>> when somebody forgets in the future. Really, the caller of > >>> > >>> download_files > >>> > >>> should just check for a successful return; we return 1 as an error > >>> from > >>> lots of > >>> > >>> functions. > >>> > >>> > >>> > >>> I'll change that too. This should still be accepted though. > >>> > >> > >> Why? If your reasoning is just that -1 is a better error value, we use 1 > >> in > >> lots of other places like I said and I don't want to change that one at a > >> time. > >> > >> $ grep 'return 1;' lib/libalpm/*.c src/*/*.c | wc -l > >> > >> > >> [0][1016] > >> 132 > >> > > > > Everywhere in the function returns -1. Lets at least be consistent for > > the same function. > > > > Not to mention download_files returns 1 on everything up to date so 1 is > not an error in this case.
find_dl_candidates, the function you are modifying, only return 0 or 1. The problem is in download_files because it does not guarantee its own return value: it's not returning 0 on success/-1 on error, it's returning <whatever the functions it calls return>. Somebody modifying those functions in the future will have a hard time understanding the significance of their return values because they have to check not just the function calling them (download_files), but also the function calling download_files. That's a good recipe for confusion and exactly this kind of bug in the future. However you want to fix this, it should be clear from looking at download_files what the significance of find_dl_candidates' return value is. apg
