some updates for PR 157

NIFTI-Imaging / nifti_clib

C libraries for NIFTI support

Other

35 stars 25 forks source link

some updates for PR 157 #171

Closed afni-rickr closed 1 year ago

afni-rickr commented 1 year ago

put variable definitions at function blocks
print int64_t using PRId64
znz_off_t varies, so try not to print
extensions are int size, limit size and check in read_file_text()
call strlen() only after NULL check
modify_field(), check length before copy
act_disp_ci() limited to int number of elements
formatting

seanm commented 1 year ago

@afni-rickr can you squash the 3 commits? Otherwise history will be weird with changes done and undone.

afni-rickr commented 1 year ago

I was going to just do this as a new PR and a single commit, unless you know of a sneakier way to go.

seanm commented 1 year ago

I was going to just do this as a new PR and a single commit, unless you know of a sneakier way to go.

You can transform this PR into a single commit by "squashing the commits". Is that something you are unfamiliar with? If so, it's not that hard, I can walk you through it...

afni-rickr commented 1 year ago

I was going to just do this as a new PR and a single commit, unless you know of a sneakier way to go.

You can transform this PR into a single commit by "squashing the commits". Is that something you are unfamiliar with? If so, it's not that hard, I can walk you through it...

Sure, I have generally avoided that. My concern is that committing a git reset --soft HEAD~3 result would require a force push, diverging with other versions of this branch (e.g. yours). How would you personally perform the squash?

seanm commented 1 year ago

I would do git rebase -i HEAD~3 and mark the last 2 commits to be squashed. Then git push -f origin HEAD to update this PR. The force is no problem because this PR isn't merged to master yet, so nothing else is based on these commits.

afni-rickr commented 1 year ago

Okay, will do. But to be sure, this would cause your local branch of update_157 to diverge, is that right? Thanks for the info.

seanm commented 1 year ago

Okay, will do. But to be sure, this would cause your local branch of update_157 to diverge, is that right? Thanks for the info.

I only have a local branch to do code review on my own computer, for big changes I find the github UI not great.

seanm commented 1 year ago

Ah, there were a few other changes I was going to suggest, but as you've already merged, I'll make a new PR...