Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH(UX): explicitly disallow empty string for --since in push #4682

Merged
merged 2 commits into from Jul 13, 2020

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Jul 7, 2020

I had forgotten that it got a new magical ^ to operate since the last published state, so ran it (datalad push --data auto --since= --to=datalad-public -r) and was confused with all the progress bars (about to transfer 140G etc... disappeared quickly though... some separate issue probably to demonstrate/file) and only then remembered:

  --since SINCE         specifies commit-ish (tag, shasum, etc.) from which to
                        look for changes to decide whether pushing is
                        necessary. If '^' is given, the last state of the
                        current branch at the sibling is taken as a starting
                        point. Constraints: value must be a string

but I wonder if we should explicitly check if value is not an empty string (not None) and fail

Positioned against maint since an empty string doesn't match documented allowed values ;-)

@kyleam
Copy link
Collaborator

kyleam commented Jul 8, 2020

In my view this is a pure bug fix.

I think ideally the check you're adding would be done a bit downstream:

if since == '^':
# figure out state of remote branch and set `since`
since = _get_corresponding_remote_state(ds_repo, to)
if not since:
lgr.info(
"No tracked remote for active branch, "
"detection of last pushed state not in effect.")
elif since:
# will blow with ValueError if unusable
ds_repo.get_hexsha(since)

We could add a separate arm for the empty string, but IMO a better option is to s/elif since/elif since is not None/ and let the empty string be handled like all other invalid values.

@yarikoptic
Copy link
Member Author

yarikoptic commented Jul 8, 2020

Sounds good, I will move or feel welcome to do so, I am unlikely to get to it today

Before the previous commit, an undocumented empty string for `since`
unintentionally had the same behavior as since=None.
@kyleam
Copy link
Collaborator

kyleam commented Jul 8, 2020

Sounds good, I will move or feel welcome to do so, I am unlikely to get to it today

Done. Of course feel free to drop any of the changes I pushed.

@@ -235,7 +231,7 @@ def __call__(
lgr.info(
"No tracked remote for active branch, "
"detection of last pushed state not in effect.")
elif since:
elif since is not None:
# will blow with ValueError if unusable
ds_repo.get_hexsha(since)
Copy link
Member Author

@yarikoptic yarikoptic Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed it does guard but I think we could improve UX with a more informative message above instead of

$> datalad push --since=''   
[ERROR  ] Unknown commit identifier:  [gitrepo.py:format_commit:1721] (ValueError) 

Copy link
Collaborator

@kyleam kyleam Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quite like that uniform treatment, but I thought you might not. As I said, feel free to drop what you don't want and add whatever extra code you'd like.

@yarikoptic yarikoptic added the merge-if-ok OP considers this work done, and requests review/merge label Jul 13, 2020
@kyleam kyleam merged commit 547acd9 into datalad:maint Jul 13, 2020
9 of 10 checks passed
@yarikoptic yarikoptic deleted the enhrf-since branch Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-if-ok OP considers this work done, and requests review/merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants