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

dcmp --sync option #42

Merged
merged 53 commits into from Jun 14, 2017

Conversation

Projects
None yet
2 participants
@dsikich
Member

dsikich commented Feb 15, 2017

  1. Added a sync option to dcmp.c
  2. Reused some code to create a dcmp_only_dst function in order to check which files were only in the dst list and copy them to the remove list.
  • note: for 2 I did that because in dcmp_strmap_compare it is looping over only the src map, and checking for files only in the src directory. So, in order to get the files that need to be deleted in the dst list I looped over the dst map. I reused some code in dcmp_strmap_check_dst, but I moved some of it to another function because it did a lot more than I thought I needed. Do you think there is a better way to get which files need to be removed?

@dsikich dsikich self-assigned this Feb 15, 2017

@dsikich dsikich requested a review from adammoody Feb 15, 2017

@adammoody

This comment has been minimized.

Show comment
Hide comment
@adammoody

adammoody Feb 16, 2017

Member

@dsikich This looks good to me. I was surprised to see that the strmap_compare only loops over the source map, too. I didn't realize that. I guess it must not report the number of files in the destination that don't exist in the source?

Member

adammoody commented Feb 16, 2017

@dsikich This looks good to me. I was surprised to see that the strmap_compare only loops over the source map, too. I didn't realize that. I guess it must not report the number of files in the destination that don't exist in the source?

@dsikich

This comment has been minimized.

Show comment
Hide comment
@dsikich

dsikich Feb 16, 2017

Member

@adammoody Okay, thank you. I was looking through it again in totalview, and it does find the number of files in the destination that don't exist in the source, but it does so after the call to dcmp_strmap_compare in main. It is on the call to dcmp_outputs_write that it finds this info, and prints it to the console. It is in dcmp_output_write. I could move all of the remove/sync logic to after the dcmp_stramp_compare function completes?

Member

dsikich commented Feb 16, 2017

@adammoody Okay, thank you. I was looking through it again in totalview, and it does find the number of files in the destination that don't exist in the source, but it does so after the call to dcmp_strmap_compare in main. It is on the call to dcmp_outputs_write that it finds this info, and prints it to the console. It is in dcmp_output_write. I could move all of the remove/sync logic to after the dcmp_stramp_compare function completes?

dsikich and others added some commits Mar 8, 2017

@adammoody adammoody added this to the v0.7 milestone May 17, 2017

@dsikich dsikich changed the title from added sync option that removes files only in dst list to --sync option in dcmp Jun 1, 2017

@dsikich dsikich changed the title from --sync option in dcmp to dcmp --sync option Jun 1, 2017

@adammoody

This comment has been minimized.

Show comment
Hide comment
@adammoody

adammoody Jun 9, 2017

Member

There are two things here I'm thinking about.

First, in the case that we hit an error when trying to compare the content of two files, we return -1. We then use that return value in the segmented scan, which is really set up to expect 0 or 1. I think it may still do the right thing with -1, but I think we should update the code to check the return value and use 1 in case 0 is not returned.

Second, a bigger issue is that I think we may not do the right thing if the source and destination files are different, but we hit an error when opening/seeking. In this case, we probably will detect the difference, but we'll have bailed out from the function when we should have copied over the destination file with the source data. So we may report the files as being different, but the sync won't have fixed them. If this is true, we should probably make sure the tool really reports an error here.

I'll work on these today.

Member

adammoody commented Jun 9, 2017

There are two things here I'm thinking about.

First, in the case that we hit an error when trying to compare the content of two files, we return -1. We then use that return value in the segmented scan, which is really set up to expect 0 or 1. I think it may still do the right thing with -1, but I think we should update the code to check the return value and use 1 in case 0 is not returned.

Second, a bigger issue is that I think we may not do the right thing if the source and destination files are different, but we hit an error when opening/seeking. In this case, we probably will detect the difference, but we'll have bailed out from the function when we should have copied over the destination file with the source data. So we may report the files as being different, but the sync won't have fixed them. If this is true, we should probably make sure the tool really reports an error here.

I'll work on these today.

@adammoody

Looked over the latest commits. Those look good to me. Go ahead and merge it! Hooray! Nice work, @dsikich.

@dsikich dsikich merged commit 3736679 into master Jun 14, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@dsikich dsikich deleted the sikich1/sync branch Jun 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment