Skip to content

Merge folders in decompression #798

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

tommady
Copy link
Contributor

@tommady tommady commented Apr 18, 2025

try to
close #466
close #727

I am very open-minded to accepting any suggestions.
The image below is just an example of fulfilling the purpose of those issues.

image

@marcospb19 marcospb19 changed the title intro merge action in decompression Merge folders in decompression Apr 18, 2025
Copy link
Member

@marcospb19 marcospb19 left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! I never before considered this could be an option in the prompt instead of the default, it seems like a good idea so I'm thinking about letting that be.

Can you add a test that unpacks two archives with the following structures:

a/b/c/1

and

a/b/c/2

And show that when decompressed and provided m via stdin, they will merge into:

a/
 b/
  c/
   1
   2

You can start from a copy of multiple_files_with_conflict_and_choice_to_rename that uses the 'allow_piped_choice' feature flag (which we should someday turn into a flag), with that you can pass the stdin choice you want.

@@ -48,49 +48,71 @@ pub enum FileConflitOperation {
/// Rename the file
/// It'll be put "_1" at the end of the filename or "_2","_3","_4".. if already exists
Rename,
/// Merge duplicated files
Merge,
Copy link
Member

Choose a reason for hiding this comment

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

We have a flag for always overwriting and a flag for always renaming, can you add a flag --merge to always merge? (without asking the prompt).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you guide me on which flag represents the always renaming?
because, from my understanding

flag --yes = always override

And yes, I can add a new flag in the decompress command to always merge.

Copy link
Member

@marcospb19 marcospb19 Apr 21, 2025

Choose a reason for hiding this comment

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

Huh I thought the rename PR added a flag for that... maybe it didn't

Copy link
Contributor Author

@tommady tommady Apr 22, 2025

Choose a reason for hiding this comment

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

hmmm, sorry, I read the question.rs deeply found out to add a new flag --merge we need to refactor the function pub fn user_wants_to_overwrite which might be beyond the PR's scope.

So maybe we should create another PR to add

  1. --merge
  2. --overwrite

WDYT

Copy link
Member

@marcospb19 marcospb19 left a comment

Choose a reason for hiding this comment

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

This is also on a good path for merging :) so don't forget a changelog entry, I think the PR title reflects this change well.

@marcospb19 marcospb19 force-pushed the main branch 7 times, most recently from 9ee3fd8 to 739dfa9 Compare April 18, 2025 20:57
@tommady
Copy link
Contributor Author

tommady commented Apr 19, 2025

Thanks for this PR! I never before considered this could be an option in the prompt instead of the default, it seems like a good idea so I'm thinking about letting that be.

Can you add a test that unpacks two archives with the following structures:

a/b/c/1

and

a/b/c/2

And show that when decompressed and provided m via stdin, they will merge into:

a/
 b/
  c/
   1
   2

You can start from a copy of multiple_files_with_conflict_and_choice_to_rename that uses the 'allow_piped_choice' feature flag (which we should someday turn into a flag), with that you can pass the stdin choice you want.

I am stuck on how to write the test. Thanks for your guidance. very appriceiate!

@marcospb19 marcospb19 force-pushed the main branch 3 times, most recently from 91103b1 to 2b9da1e Compare April 21, 2025 02:05
@tommady tommady marked this pull request as ready for review April 22, 2025 04:52
Signed-off-by: tommady <[email protected]>
@tommady tommady requested a review from marcospb19 April 24, 2025 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants