Tools: Show warning when using rsync with untracked files #41442

tbradsha · 2025-01-30T22:10:58Z

By design jetpack rsync only syncs files that are expected to be in production. This means untracked files are not synced. However, this has caused confusion in the past (e.g. p1737037284874559-slack-CDLH4C1UZ).

This PR adds a warning prompt prior to running the rsync:

$ jetpack rsync --watch jetpack somedest
Alias found, using dest: somedest:htdocs/wp-content/mu-plugins
The below untracked files were detected in your plugin path:

  projects/plugins/jetpack/asdf asdf copy.asdf
  projects/plugins/jetpack/asdf asdf.asdf
  projects/plugins/jetpack/asdf.asdf

✔ Untracked files will not be synced. Proceed? (y/N) · false

Also, when jetpack rsync --watch is used, it will output a notice if a sync is triggered by a file that will not be synced itself for one of the following reasons:

Untracked
Excluded from production
Some unknown other reason

It's still good to trigger the sync in these cases, as more than one file might have changed.

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

Try jetpack rsync and jetpack rsync --watch with various combinations:

From the monorepo root as well as other folders
With untracked files and files excluded from production

github-actions · 2025-01-30T22:11:55Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add a "[Type]" label (Bug, Enhancement, Janitorial, Task).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Follow this PR Review Process:

Ensure all required checks appearing at the bottom of this PR are passing.
Choose a review path based on your changes:
- A. Team Review: add the "[Status] Needs Team Review" label
  - For most changes, including minor cross-team impacts.
  - Example: Updating a team-specific component or a small change to a shared library.
- B. Crew Review: add the "[Status] Needs Review" label
  - For significant changes to core functionality.
  - Example: Major updates to a shared library or complex features.
- C. Both: Start with Team, then request Crew
  - For complex changes or when you need extra confidence.
  - Example: Refactor affecting multiple systems.
Get at least one approval before merging.

Still unsure? Reach out in #jetpack-developers for guidance!

anomiex

Seems to work fine. Some comments inline about the warning with --watch though.

anomiex · 2025-01-31T16:49:30Z

tools/cli/commands/rsync.js

@@ -86,6 +104,22 @@ export async function rsyncInit( argv ) {

 			const paths = await rsyncToDest( sourcePluginPath, finalDest );

+			// Warn but don't fail if file was intentionally not synced.


Except sometimes being triggered by unsynced file changes is intentional.

jetpack/tools/cli/commands/rsync.js

Line 139 in af2df47

// Also watch the git index for changes to catch `git add`, as that may change which files are synced.

jetpack/tools/cli/commands/rsync.js

Lines 142 to 143 in af2df47

// Watch `.gitignore` and `.gitattributes` in parent dirs, as they too may change which files are synced.

// Here we assume sourcePluginPath is always `projects/plugins/whatever`

The same could apply to changes in .gitignore or .gitattributes in the plugin dir itself.

Your change here doesn't break it, since it still does the sync, but it does print a potentially confusing message. After a git add, for example,

building file list ... 543 files to consider foo.txt 0 100% 0.00kB/s 0:00:00 (xfr#1, to-chk=536/543) sent 15,131 bytes received 34 bytes 30,330.00 bytes/sec total size is 2,560,591 speedup is 168.85 Sync was triggered by a change to '../../../.git/index', but it was not synced. Reason: not tracked by git

Also of note is that eventfile may not be the only file that changed. We debounce the events, so if several files are changed in quick succession only the last one will be seen here. Again, that might be confusing if the last is untracked but earlier ones were tracked.

Except sometimes being triggered by unsynced file changes is intentional.

Also of note is that eventfile may not be the only file that changed. We debounce the events, so if several files are changed in quick succession only the last one will be seen here. Again, that might be confusing if the last is untracked but earlier ones were tracked.

Yes, I alluded to that in the PR body: "It's still good to trigger the sync in these cases, as more than one file might have changed."

I believe the message is technically accurate, but am open to suggestions on how to make it less confusing.

What's the purpose of the warning? If we want to repeat the "these untracked files weren't synced" warning, maybe just do that directly?

There are two parts:

Highlight untracked files in case someone forgot to git add. The user can proceed or abort at this stage.

When a file is changed during watch, it might not be synced. Sometimes that's expected, and sometimes it's not.

I think your concern lies solely on the watch scenario, correct?

We could show a prompt each time (if that's what you mean), but that somewhat defeats the point of a background watch, particularly when it could be something we know about and don't care about.

Aside: In cases when the file is arbitrarily changed in a batch of changes, it's likely that file's event will be debounced and not show the notice anyway, but if it's individually edited, there's a larger chance that the user is trying to get it synced and it's failing.

Two tweaks I can think of while fulfilling the above:

Put the notices behind the -v flag.

Filter out changes to a few hard-coded files (e.g. ../../../.git/index).

Yes, the watch scenario is what I'm concerned with. And yeah, definitely no prompt, that would defeat the purpose.

One idea I had is to keep a list of untracked files from the last warning, and at each watch-sync warn about any new untracked files that showed up, like "The following new untracked files are not being synced". Although that would miss the case where someone keeps saving the same file because they didn't see an earlier warning. Maybe only print eventfile if it's in the list of untracked files and there are no new ones?

Discussed this in Slack; there's not one good answer as written. For now I've moved the notices to only show in verbose mode: 2d7cbae

anomiex

Good enough. 🚢

Show warning when using rsync with untracked files

af2df47

tbradsha added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Tools] Development CLI The tools/cli to assist during JP development. labels Jan 30, 2025

tbradsha requested a review from a team January 30, 2025 22:10

tbradsha self-assigned this Jan 30, 2025

github-actions bot added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 30, 2025

anomiex reviewed Jan 31, 2025

View reviewed changes

kraftbj mentioned this pull request Feb 6, 2025

Add note about git add for rsync tracking #41605

Merged

3 tasks

tbradsha added 2 commits February 7, 2025 12:59

Don't ever warn about some specific files

426b4c4

Put notice under verbose flag

2d7cbae

tbradsha requested a review from anomiex February 7, 2025 21:00

anomiex approved these changes Feb 7, 2025

View reviewed changes

tbradsha merged commit 46129be into trunk Feb 7, 2025
59 checks passed

tbradsha deleted the add/tools/show_untracked_files_warning_on_rsync branch February 7, 2025 21:21

github-actions bot removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Feb 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tools: Show warning when using rsync with untracked files #41442

Tools: Show warning when using rsync with untracked files #41442

tbradsha commented Jan 30, 2025

github-actions bot commented Jan 30, 2025 •

edited

Loading

anomiex left a comment

anomiex Jan 31, 2025

tbradsha Jan 31, 2025

anomiex Jan 31, 2025

tbradsha Jan 31, 2025

anomiex Jan 31, 2025

tbradsha Feb 7, 2025

anomiex left a comment

		@@ -86,6 +104,22 @@ export async function rsyncInit( argv ) {

		const paths = await rsyncToDest( sourcePluginPath, finalDest );

		// Warn but don't fail if file was intentionally not synced.

	// Watch `.gitignore` and `.gitattributes` in parent dirs, as they too may change which files are synced.
	// Here we assume sourcePluginPath is always `projects/plugins/whatever`

Tools: Show warning when using rsync with untracked files #41442

Tools: Show warning when using rsync with untracked files #41442

Conversation

tbradsha commented Jan 30, 2025

Other information:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented Jan 30, 2025 • edited Loading

anomiex left a comment

Choose a reason for hiding this comment

anomiex Jan 31, 2025

Choose a reason for hiding this comment

tbradsha Jan 31, 2025

Choose a reason for hiding this comment

anomiex Jan 31, 2025

Choose a reason for hiding this comment

tbradsha Jan 31, 2025

Choose a reason for hiding this comment

anomiex Jan 31, 2025

Choose a reason for hiding this comment

tbradsha Feb 7, 2025

Choose a reason for hiding this comment

anomiex left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 30, 2025 •

edited

Loading