Repository: skypilot-org/skypilot
Pull request: [Storage][Spot] Resolving failure to exclude local files for managed spot jobs using git --ignored outputs
This PR closes #1898 and opposed to implementing our own `.gitignore` filter in #1939 and using `rsync` from #1993 , this harness `git -C /path/to/source/dir status --ignored -s` to get a list of files/directories to be excluded from syncing `sky storage`.

Added the testing structure and the files supposed to be ignored from syncing divided into `git_info_exclude_test`(corresponding to `.git/info/exclude`) and `gitignore_test`(`.gitignore`).

<!-- Describe the tests ran -->
<!-- Unit tests (tests/test_*.py) are part of GitHub CI; below are tests that launch on the cloud. -->

Tested (run the relevant ones):
- [x] Manually ran `sky spot launch` and checked the `workdir` and local `file_mounts` src from the spot instance
- [x] Relevant individual smoke tests: `pytest tests/test_smoke.py::TestStorageWithCredentials --cloudflare` 
- [x] Relevant individual smoke tests: `pytest tests/test_smoke.py --managed-spot`
- [x] Additional tests for managed spot jobs added to `test_smoke.py/test_excluded_file_cloud_storage_upload_copy`


Problem statement:
Spot launch doesn't respect `.gitignore`
When launching an instance with `sky launch`, files included in the my `.gitignore` file are ignored during the rsync. However, when I launch a job with `sky spot launch`, it seems as though the files are included. Is this intended behavior?

Hints from issue discussion:
Thanks for reporting the issue @keirp! This has been an issue for a while. The inconsistency comes from the cloud bucket’s rsync interface misalign with the system’s rsync. As we are using cloud bucket as the backend for the files uploading to the spot job, instead of using the system’s rsync for normal jobs, the problem occurs. cc @romilb, maybe we can manually list all the files to be uploaded using rsync’s —dryrun and call the cloud bucket cli with a list of files?
________________________________
From: keirp ***@***.***>
Sent: Tuesday, April 25, 2023 1:00:17 PM
To: skypilot-org/skypilot ***@***.***>
Cc: Subscribed ***@***.***>
Subject: [skypilot-org/skypilot] Spot launch doesn't respect `.gitignore` (Issue #1898)


When launching an instance with sky launch, files included in the my .gitignore file are ignored during the rsync. However, when I launch a job with sky spot launch, it seems as though the files are included. Is this intended behavior?

—
Reply to this email directly, view it on GitHub<https://github.com/skypilot-org/skypilot/issues/1898>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABTQXJM3B7EHZDFZWHX7BHTXDAUNDANCNFSM6AAAAAAXLPCJG4>.
You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

cc @romilbhardwaj 

Another idea is to read `.gitignore` and pass each line as an `--exclude` arg to the `aws s3 sync` command: https://github.com/skypilot-org/skypilot/blob/master/sky/data/storage.py#L1711
Hit this myself. Current workdir has a bunch of "results" non-code files that should not be uploaded. Now every `spot launch` is taking quite a while just for uploading these. An important UX issue.
I'll take on this one.
@concretevitamin Would you mind elaborating on what you meant by 'bunch of "results" non-code files'? I haven't encountered anything odd that got uploaded to the spot instance while running `spot launch`
I think he was just providing an example - he has a some non-code files and dirs (e.g., experiment results) that are large and included in his gitignore, but they still get uploaded when running `sky spot launch`. If we parse .gitignore correctly when uploading storage objects, these files should not get uploaded.

Commit message: gitignore filter using git

Patch to review:
diff --git a/sky/data/storage.py b/sky/data/storage.py
index 1c3420ad869..a5f25ad1512 100644
--- a/sky/data/storage.py
+++ b/sky/data/storage.py
@@ -873,6 +873,75 @@ def add_if_not_none(key: str, value: Optional[Any]):
         return config
 
 
+def format_gitignore_to_exclude_list(src_dir_path: str) -> List[str]:
+    """Returns a list of formatted excluded files read from .gitignore and
+    .git/info/exclude using git. 'git init' is ran when SRC_DIR_PATH
+    is not a git repository and removed after obtaining excluded list
+    """
+    expand_src_dir_path = os.path.expanduser(src_dir_path)
+
+    git_exclude_path = os.path.join(expand_src_dir_path, '.git/info/exclude')
+    gitignore_path = os.path.join(expand_src_dir_path, '.gitignore')
+
+    git_exclude_exists = os.path.isfile(git_exclude_path)
+    gitignore_exists = os.path.isfile(gitignore_path)
+
+    # This command outputs a list to be excluded according to .gitignore
+    # and .git/info/exclude
+    filter_cmd = f'git -C {expand_src_dir_path} status --ignored -s'
+    excluded_list: List[str] = ['.git/*', '.gitignore']
+
+    # pylint: disable=W1510
+    if git_exclude_exists or gitignore_exists:
+        try:
+            output = subprocess.run(filter_cmd,
+                                    shell=True,
+                                    stdout=subprocess.PIPE,
+                                    stderr=subprocess.PIPE,
+                                    check=True,
+                                    text=True)
+        except subprocess.CalledProcessError as e:
+            # when the SRC_DIR_PATH is not a git repo
+            if e.returncode == exceptions.GIT_UNINITIALIZED_CODE:
+                init_cmd = f'git -C {expand_src_dir_path} init'
+                subprocess.run(init_cmd,
+                               shell=True,
+                               stdout=subprocess.PIPE,
+                               stderr=subprocess.PIPE)
+                output = subprocess.run(filter_cmd,
+                                        shell=True,
+                                        stdout=subprocess.PIPE,
+                                        stderr=subprocess.PIPE,
+                                        text=True)
+                if git_exclude_exists:
+                    # removes all the files/dirs created with 'git init'
+                    # under .git/ except .git/info/exclude
+                    remove_files_cmd = (f'find {expand_src_dir_path}/.git ' \
+                                        f'-path {git_exclude_path} -prune -o ' \
+                                        '-type f -exec rm -f {} +')
+                    remove_dirs_cmd = (f'find {expand_src_dir_path}/.git ' \
+                                       f'-path {git_exclude_path} -prune -o' \
+                                       ' -type d -empty -delete')
+                    subprocess.run(remove_files_cmd,
+                                   shell=True,
+                                   stdout=subprocess.PIPE,
+                                   stderr=subprocess.PIPE)
+                    subprocess.run(remove_dirs_cmd,
+                                   shell=True,
+                                   stdout=subprocess.PIPE,
+                                   stderr=subprocess.PIPE)
+
+        output_list = output.stdout.split('\n')
+        for line in output_list:
+            if line.startswith('!!'):
+                to_be_excluded = line[3:]
+                if line.endswith('/'):
+                    to_be_excluded += '*'
+                excluded_list.append(to_be_excluded)
+    # pylint: enable=W1510
+    return excluded_list
+
+
 class S3Store(AbstractStore):
     """S3Store inherits from Storage Object and represents the backend
     for S3 buckets.
@@ -1075,10 +1144,12 @@ def get_file_sync_command(base_dir_path, file_names):
 
         def get_dir_sync_command(src_dir_path, dest_dir_name):
             # we exclude .git directory from the sync
-            sync_command = (
-                'aws s3 sync --no-follow-symlinks --exclude ".git/*" '
-                f'{src_dir_path} '
-                f's3://{self.name}/{dest_dir_name}')
+            excluded_list = format_gitignore_to_exclude_list(src_dir_path)
+            excludes = ' '.join(
+                [f'--exclude "{file_name}"' for file_name in excluded_list])
+            sync_command = (f'aws s3 sync --no-follow-symlinks {excludes} '
+                            f'{src_dir_path} '
+                            f's3://{self.name}/{dest_dir_name}')
             return sync_command
 
         # Generate message for upload
@@ -1498,8 +1569,10 @@ def get_file_sync_command(base_dir_path, file_names):
 
         def get_dir_sync_command(src_dir_path, dest_dir_name):
             # we exclude .git directory from the sync
-            sync_command = (f'gsutil -m rsync -r -x \'.git/*\' {src_dir_path} '
-                            f'gs://{self.name}/{dest_dir_name}')
+            excluded_list = format_gitignore_to_exclude_list(src_dir_path)
+            excludes = '\'(' + '|'.join(excluded_list) + ')\''
+            sync_command = (f'gsutil -m rsync -r -x {excludes} {src_dir_path}'
+                            f' gs://{self.name}/{dest_dir_name}')
             return sync_command
 
         # Generate message for upload
@@ -1821,15 +1894,18 @@ def get_file_sync_command(base_dir_path, file_names):
 
         def get_dir_sync_command(src_dir_path, dest_dir_name):
             # we exclude .git directory from the sync
+            excluded_list = format_gitignore_to_exclude_list(src_dir_path)
+            excludes = ' '.join(
+                [f'--exclude "{file_name}"' for file_name in excluded_list])
             endpoint_url = cloudflare.create_endpoint()
-            sync_command = (
-                'AWS_SHARED_CREDENTIALS_FILE='
-                f'{cloudflare.R2_CREDENTIALS_PATH} '
-                'aws s3 sync --no-follow-symlinks --exclude ".git/*" '
-                f'{src_dir_path} '
-                f's3://{self.name}/{dest_dir_name} '
-                f'--endpoint {endpoint_url} '
-                f'--profile={cloudflare.R2_PROFILE_NAME}')
+
+            sync_command = ('AWS_SHARED_CREDENTIALS_FILE='
+                            f'{cloudflare.R2_CREDENTIALS_PATH} '
+                            f'aws s3 sync --no-follow-symlinks {excludes} '
+                            f'{src_dir_path} '
+                            f's3://{self.name}/{dest_dir_name} '
+                            f'--endpoint {endpoint_url} '
+                            f'--profile={cloudflare.R2_PROFILE_NAME}')
             return sync_command
 
         # Generate message for upload
diff --git a/sky/exceptions.py b/sky/exceptions.py
index 3dfb7172d17..3e1e1e34752 100644
--- a/sky/exceptions.py
+++ b/sky/exceptions.py
@@ -12,6 +12,8 @@
 RSYNC_FILE_NOT_FOUND_CODE = 23
 # Arbitrarily chosen value. Used in SkyPilot's storage mounting scripts
 MOUNT_PATH_NON_EMPTY_CODE = 42
+# Return code when git command is ran in a dir that is not git repo
+GIT_UNINITIALIZED_CODE = 128
 
 
 class ResourcesUnavailableError(Exception):
diff --git a/tests/git_info_exclude_test b/tests/git_info_exclude_test
new file mode 100644
index 00000000000..801578d7cd5
--- /dev/null
+++ b/tests/git_info_exclude_test
@@ -0,0 +1,8 @@
+question_mark/excluded?.txt
+square_bracket/excluded[0-9].txt
+square_bracket_single/excluded[01].txt
+square_bracket_excla/excluded[!01].txt
+square_bracket_alpha/excluded[a-z].txt
+excluded_dir/
+nested_double_asterisk/**/also_exclude.txt
+nested_wildcard_dir/*day/also_exclude.txt
diff --git a/tests/gitignore_test b/tests/gitignore_test
new file mode 100644
index 00000000000..2eab88de363
--- /dev/null
+++ b/tests/gitignore_test
@@ -0,0 +1,11 @@
+# This test should upload only include.txt, included.log, and included/included.l$
+**/double_asterisk_excluded
+**/double_asterisk_excluded_dir
+**/parent/*.txt
+**/parent/child/*.txt
+*.log
+exp-*/
+!included.log
+excluded.*
+/front_slash_excluded
+no_slash_excluded
diff --git a/tests/test_smoke.py b/tests/test_smoke.py
index 6756c5d0789..fbc5e456d1a 100644
--- a/tests/test_smoke.py
+++ b/tests/test_smoke.py
@@ -25,6 +25,7 @@
 import hashlib
 import inspect
 import pathlib
+import shutil
 import subprocess
 import sys
 import tempfile
@@ -1911,6 +1912,96 @@ class TestStorageWithCredentials:
         'abc_',  # ends with an underscore
     ]
 
+    GITIGNORE_SYNC_TEST_DIR_STRUCTURE = {
+        'double_asterisk': {
+            'double_asterisk_excluded': None,
+            'double_asterisk_excluded_dir': {
+                'dir_excluded': None,
+            },
+        },
+        'double_asterisk_parent': {
+            'parent': {
+                'also_excluded.txt': None,
+                'child': {
+                    'double_asterisk_parent_child_excluded.txt': None,
+                },
+                'double_asterisk_parent_excluded.txt': None,
+            },
+        },
+        'excluded.log': None,
+        'excluded_dir': {
+            'excluded.txt': None,
+            'nested_excluded': {
+                'excluded': None,
+            },
+        },
+        'exp-1': {
+            'be_excluded': None,
+        },
+        'exp-2': {
+            'be_excluded': None,
+        },
+        'front_slash_excluded': None,
+        'included.log': None,
+        'included.txt': None,
+        'include_dir': {
+            'excluded.log': None,
+            'included.log': None,
+        },
+        'nested_double_asterisk': {
+            'one': {
+                'also_exclude.txt': None,
+            },
+            'two': {
+                'also_exclude.txt': None,
+            },
+        },
+        'nested_wildcard_dir': {
+            'monday': {
+                'also_exclude.txt': None,
+            },
+            'tuesday': {
+                'also_exclude.txt': None,
+            },
+        },
+        'no_slash_excluded': None,
+        'no_slash_tests': {
+            'no_slash_excluded': {
+                'also_excluded.txt': None,
+            },
+        },
+        'question_mark': {
+            'excluded1.txt': None,
+            'excluded@.txt': None,
+        },
+        'square_bracket': {
+            'excluded1.txt': None,
+        },
+        'square_bracket_alpha': {
+            'excludedz.txt': None,
+        },
+        'square_bracket_excla': {
+            'excluded2.txt': None,
+            'excluded@.txt': None,
+        },
+        'square_bracket_single': {
+            'excluded0.txt': None,
+        },
+    }
+
+    @staticmethod
+    def create_dir_structure(base_path, structure):
+        for name, substructure in structure.items():
+            path = os.path.join(base_path, name)
+            if substructure is None:
+                # Create a file
+                open(path, 'a').close()
+            else:
+                # Create a subdirectory
+                os.mkdir(path)
+                TestStorageWithCredentials.create_dir_structure(
+                    path, substructure)
+
     @staticmethod
     def cli_delete_cmd(store_type, bucket_name):
         if store_type == storage_lib.StoreType.S3:
@@ -1946,6 +2037,35 @@ def cli_ls_cmd(store_type, bucket_name, suffix=''):
                 url = f's3://{bucket_name}'
             return f'AWS_SHARED_CREDENTIALS_FILE={cloudflare.R2_CREDENTIALS_PATH} aws s3 ls {url} --endpoint {endpoint_url} --profile=r2'
 
+    @staticmethod
+    def cli_count_name_in_bucket(store_type, bucket_name, file_name, suffix=''):
+        if store_type == storage_lib.StoreType.S3:
+            if suffix:
+                return f'aws s3api list-objects --bucket "{bucket_name}" --prefix {suffix} --query "length(Contents[?contains(Key,\'{file_name}\')].Key)"'
+            else:
+                return f'aws s3api list-objects --bucket "{bucket_name}" --query "length(Contents[?contains(Key,\'{file_name}\')].Key)"'
+        elif store_type == storage_lib.StoreType.GCS:
+            if suffix:
+                return f'gsutil ls gs://{bucket_name}/{suffix} | grep "{file_name}" | wc -l'
+            else:
+                return f'gsutil ls gs://{bucket_name} | grep "{file_name}" | wc -l'
+        elif store_type == storage_lib.StoreType.R2:
+            endpoint_url = cloudflare.create_endpoint()
+            if suffix:
+                return f'AWS_SHARED_CREDENTIALS_FILE={cloudflare.R2_CREDENTIALS_PATH} aws s3api list-objects --bucket "{bucket_name}" --prefix {suffix} --query "length(Contents[?contains(Key,\'{file_name}\')].Key)" --endpoint {endpoint_url} --profile=r2'
+            else:
+                return f'AWS_SHARED_CREDENTIALS_FILE={cloudflare.R2_CREDENTIALS_PATH} aws s3api list-objects --bucket "{bucket_name}" --query "length(Contents[?contains(Key,\'{file_name}\')].Key)" --endpoint {endpoint_url} --profile=r2'
+
+    @staticmethod
+    def cli_count_file_in_bucket(store_type, bucket_name):
+        if store_type == storage_lib.StoreType.S3:
+            return f'aws s3 ls s3://{bucket_name} --recursive | wc -l'
+        elif store_type == storage_lib.StoreType.GCS:
+            return f'gsutil ls -r gs://{bucket_name}/** | wc -l'
+        elif store_type == storage_lib.StoreType.R2:
+            endpoint_url = cloudflare.create_endpoint()
+            return f'AWS_SHARED_CREDENTIALS_FILE={cloudflare.R2_CREDENTIALS_PATH} aws s3 ls s3://{bucket_name} --recursive --endpoint {endpoint_url} --profile=r2 | wc -l'
+
     @pytest.fixture
     def tmp_source(self, tmp_path):
         # Creates a temporary directory with a file in it
@@ -2291,6 +2411,85 @@ def test_invalid_names(self, invalid_name_list, store_type):
                 storage_obj = storage_lib.Storage(name=name)
                 storage_obj.add_store(store_type)
 
+    @pytest.mark.parametrize(
+        'gitignore_structure, store_type',
+        [(GITIGNORE_SYNC_TEST_DIR_STRUCTURE, storage_lib.StoreType.S3),
+         (GITIGNORE_SYNC_TEST_DIR_STRUCTURE, storage_lib.StoreType.GCS),
+         pytest.param(GITIGNORE_SYNC_TEST_DIR_STRUCTURE,
+                      storage_lib.StoreType.R2,
+                      marks=pytest.mark.cloudflare)])
+    def test_excluded_file_cloud_storage_upload_copy(self, gitignore_structure,
+                                                     store_type):
+        # tests if files included in .gitignore and .git/info/exclude are
+        # excluded from being transferred to Storage
+
+        with tempfile.TemporaryDirectory() as tmpdir:
+            # Create files to be uploaded in the Storage
+            self.create_dir_structure(tmpdir, gitignore_structure)
+
+            # Create files to be excluded from .gitignore and list them in the file
+            skypilot_path = os.path.abspath(sys.argv[0])
+            skypilot_path = os.path.dirname(skypilot_path)
+            temp_path = f'{tmpdir}/.gitignore'
+            file_path = os.path.join(skypilot_path, 'tests/gitignore_test')
+            shutil.copyfile(file_path, temp_path)
+
+            # Create files to be excluded from .git/info/exclude and list them in the file
+            temp_path = f'{tmpdir}/.git/info/'
+            os.makedirs(temp_path)
+            temp_exclude_path = os.path.join(temp_path, 'exclude')
+            file_path = os.path.join(skypilot_path,
+                                     'tests/git_info_exclude_test')
+            shutil.copyfile(file_path, temp_exclude_path)
+
+            # Create sky Storage with the files created
+            timestamp = str(time.time()).replace('.', '')
+            bucket_name = f'sky-test-{timestamp}'
+            store_obj = storage_lib.Storage(name=bucket_name,
+                                            source=tmpdir,
+                                            mode=storage_lib.StorageMode.COPY)
+            store_obj.add_store(store_type)
+
+        upload_file_name = 'included'
+        # Count the number of files with the given file name
+        up_cmd = self.cli_count_name_in_bucket(store_type, \
+            bucket_name, file_name=upload_file_name)
+        gitignore_cmd = self.cli_count_name_in_bucket(store_type, \
+            bucket_name, file_name='.gitignore')
+        git_exclude_cmd = self.cli_count_name_in_bucket(store_type, \
+            bucket_name, file_name='.git')
+        cnt_num_file_cmd = self.cli_count_file_in_bucket(
+            store_type, bucket_name)
+
+        up_output = subprocess.check_output(up_cmd, shell=True)
+        gitignore_output = subprocess.check_output(gitignore_cmd, shell=True)
+        git_exclude_output = subprocess.check_output(git_exclude_cmd,
+                                                     shell=True)
+        cnt_output = subprocess.check_output(cnt_num_file_cmd, shell=True)
+
+        if store_type == storage_lib.StoreType.GCS:
+            up_dir_cmd = up_cmd = self.cli_count_name_in_bucket(store_type, \
+                bucket_name, file_name=upload_file_name, suffix='include_dir')
+            up_dir_output = subprocess.check_output(up_dir_cmd, shell=True)
+
+        # GCS ls and aws s3 list-objects outputs in a different way
+        # Only 'included.*' files should exist in the cloud object storage
+        if store_type == storage_lib.StoreType.GCS:
+            assert '2' in up_output.decode('utf-8'), \
+                    f'Filese to be uploaded are not uploaded.: {bucket_name}'
+            assert '1' in up_dir_output.decode('utf-8'), \
+                    f'File to be uploaded in include_dir is not uploaded.: {bucket_name}'
+        else:
+            assert '3' in up_output.decode('utf-8'), \
+                    f'{upload_file_name} is not uploaded.: {bucket_name}'
+        assert '0' in gitignore_output.decode('utf-8'), \
+               '.gitignore file should not be uploaded'
+        assert '0' in git_exclude_output.decode('utf-8'), \
+               '.git directory should not be uploaded'
+        ls_cmd = self.cli_ls_cmd(store_type, bucket_name)
+        assert '3' in cnt_output.decode('utf-8'), \
+               f'Some items listed in .gitignore and .git/info/exclude are not ignored - there must be one item in the bucket. Check with {ls_cmd}'
+
 
 # ---------- Testing YAML Specs ----------
 # Our sky storage requires credentials to check the bucket existance when


Code review comments:
diff_hunk: @@ -873,6 +873,75 @@ def add_if_not_none(key: str, value: Optional[Any]):
         return config
 
 
+def format_gitignore_to_exclude_list(src_dir_path: str) -> List[str]:
path: sky/data/storage.py
text: @user1:
minor nit: Consider renaming to `get_excluded_files_from_gitignore`

diff_hunk: @@ -873,6 +873,75 @@ def add_if_not_none(key: str, value: Optional[Any]):
         return config
 
 
+def format_gitignore_to_exclude_list(src_dir_path: str) -> List[str]:
+    """Returns a list of formatted excluded files read from .gitignore and
+    .git/info/exclude using git. 'git init' is ran when SRC_DIR_PATH
+    is not a git repository and removed after obtaining excluded list
+    """
+    expand_src_dir_path = os.path.expanduser(src_dir_path)
+
+    git_exclude_path = os.path.join(expand_src_dir_path, '.git/info/exclude')
+    gitignore_path = os.path.join(expand_src_dir_path, '.gitignore')
+
+    git_exclude_exists = os.path.isfile(git_exclude_path)
+    gitignore_exists = os.path.isfile(gitignore_path)
+
+    # This command outputs a list to be excluded according to .gitignore
+    # and .git/info/exclude
+    filter_cmd = f'git -C {expand_src_dir_path} status --ignored -s'
+    excluded_list: List[str] = ['.git/*', '.gitignore']
+
+    # pylint: disable=W1510
path: sky/data/storage.py
text: @user1:
Is there a strong reason to disable W1510? Instead, should we add `check=True` for L907, L911, L925 and L930? Good to explicitly handle any errors that may occur when running them.

@author:
Origianally, I thought the commands were causing inevitable errors, but found that it actually had some issues. They are currently fixed and removed `# pylint: disable=W1510` as well.

diff_hunk: @@ -873,6 +873,75 @@ def add_if_not_none(key: str, value: Optional[Any]):
         return config
 
 
+def format_gitignore_to_exclude_list(src_dir_path: str) -> List[str]:
+    """Returns a list of formatted excluded files read from .gitignore and
+    .git/info/exclude using git. 'git init' is ran when SRC_DIR_PATH
+    is not a git repository and removed after obtaining excluded list
+    """
path: sky/data/storage.py
text: @user1:
```suggestion
    """ Lists files and patterns ignored by git in the source directory
    
    Runs `git status --ignored` which returns a list of excluded files and patterns 
    read from .gitignore and .git/info/exclude using git. `git init` is run if SRC_DIR_PATH
    is not a git repository and removed after obtaining excluded list.
    
    Returns: List[str] containing files and patterns to be ignored. <Describe the kinds of patterns, e.g., /*>
    """
```

diff_hunk: @@ -873,6 +873,75 @@ def add_if_not_none(key: str, value: Optional[Any]):
         return config
 
 
+def format_gitignore_to_exclude_list(src_dir_path: str) -> List[str]:
path: sky/data/storage.py
text: @user1:
nit: can we move this method to storage_utils.py?

diff_hunk: @@ -12,6 +12,8 @@
 RSYNC_FILE_NOT_FOUND_CODE = 23
 # Arbitrarily chosen value. Used in SkyPilot's storage mounting scripts
 MOUNT_PATH_NON_EMPTY_CODE = 42
+# Return code when git command is ran in a dir that is not git repo
+GIT_UNINITIALIZED_CODE = 128
path: sky/exceptions.py
text: @user1:
128 seems to be the default fatal exit code used everywhere in git: https://github.com/search?q=repo%3Agit%2Fgit+exit%28128%29+language%3AC&type=code

Should we rename this to something else, perhaps `GIT_FATAL_CODE`?

diff_hunk: @@ -873,6 +873,75 @@ def add_if_not_none(key: str, value: Optional[Any]):
         return config
 
 
+def format_gitignore_to_exclude_list(src_dir_path: str) -> List[str]:
+    """Returns a list of formatted excluded files read from .gitignore and
+    .git/info/exclude using git. 'git init' is ran when SRC_DIR_PATH
+    is not a git repository and removed after obtaining excluded list
+    """
+    expand_src_dir_path = os.path.expanduser(src_dir_path)
+
+    git_exclude_path = os.path.join(expand_src_dir_path, '.git/info/exclude')
+    gitignore_path = os.path.join(expand_src_dir_path, '.gitignore')
+
+    git_exclude_exists = os.path.isfile(git_exclude_path)
+    gitignore_exists = os.path.isfile(gitignore_path)
+
+    # This command outputs a list to be excluded according to .gitignore
+    # and .git/info/exclude
+    filter_cmd = f'git -C {expand_src_dir_path} status --ignored -s'
+    excluded_list: List[str] = ['.git/*', '.gitignore']
+
+    # pylint: disable=W1510
+    if git_exclude_exists or gitignore_exists:
+        try:
+            output = subprocess.run(filter_cmd,
+                                    shell=True,
+                                    stdout=subprocess.PIPE,
+                                    stderr=subprocess.PIPE,
+                                    check=True,
+                                    text=True)
+        except subprocess.CalledProcessError as e:
+            # when the SRC_DIR_PATH is not a git repo
+            if e.returncode == exceptions.GIT_UNINITIALIZED_CODE:
path: sky/data/storage.py
text: @user1:
See comment below - since 128 is the default fatal code,  should we also check for the string `not a git repository`?
```
(base) ➜  ~ git status
fatal: not a git repository (or any of the parent directories): .git
```

diff_hunk: @@ -873,6 +873,75 @@ def add_if_not_none(key: str, value: Optional[Any]):
         return config
 
 
+def format_gitignore_to_exclude_list(src_dir_path: str) -> List[str]:
+    """Returns a list of formatted excluded files read from .gitignore and
+    .git/info/exclude using git. 'git init' is ran when SRC_DIR_PATH
+    is not a git repository and removed after obtaining excluded list
+    """
+    expand_src_dir_path = os.path.expanduser(src_dir_path)
+
+    git_exclude_path = os.path.join(expand_src_dir_path, '.git/info/exclude')
+    gitignore_path = os.path.join(expand_src_dir_path, '.gitignore')
+
+    git_exclude_exists = os.path.isfile(git_exclude_path)
+    gitignore_exists = os.path.isfile(gitignore_path)
+
+    # This command outputs a list to be excluded according to .gitignore
+    # and .git/info/exclude
+    filter_cmd = f'git -C {expand_src_dir_path} status --ignored -s'
+    excluded_list: List[str] = ['.git/*', '.gitignore']
+
+    # pylint: disable=W1510
+    if git_exclude_exists or gitignore_exists:
+        try:
+            output = subprocess.run(filter_cmd,
+                                    shell=True,
+                                    stdout=subprocess.PIPE,
+                                    stderr=subprocess.PIPE,
+                                    check=True,
+                                    text=True)
+        except subprocess.CalledProcessError as e:
+            # when the SRC_DIR_PATH is not a git repo
+            if e.returncode == exceptions.GIT_UNINITIALIZED_CODE:
+                init_cmd = f'git -C {expand_src_dir_path} init'
+                subprocess.run(init_cmd,
path: sky/data/storage.py
text: @user1:
This may fail due to different reasons (e.g., user has read-only permissions in the dir). In that case, can we raise a warning and gracefully fall back to not excluding any files from .gitignore? 

@author:
Will be printing a `logger.wanring()` message and return the default `excluded_list` with `.git/*` in it.

diff_hunk: @@ -873,6 +873,75 @@ def add_if_not_none(key: str, value: Optional[Any]):
         return config
 
 
+def format_gitignore_to_exclude_list(src_dir_path: str) -> List[str]:
+    """Returns a list of formatted excluded files read from .gitignore and
+    .git/info/exclude using git. 'git init' is ran when SRC_DIR_PATH
+    is not a git repository and removed after obtaining excluded list
+    """
+    expand_src_dir_path = os.path.expanduser(src_dir_path)
+
+    git_exclude_path = os.path.join(expand_src_dir_path, '.git/info/exclude')
+    gitignore_path = os.path.join(expand_src_dir_path, '.gitignore')
+
+    git_exclude_exists = os.path.isfile(git_exclude_path)
+    gitignore_exists = os.path.isfile(gitignore_path)
+
+    # This command outputs a list to be excluded according to .gitignore
+    # and .git/info/exclude
+    filter_cmd = f'git -C {expand_src_dir_path} status --ignored -s'
+    excluded_list: List[str] = ['.git/*', '.gitignore']
+
+    # pylint: disable=W1510
+    if git_exclude_exists or gitignore_exists:
+        try:
+            output = subprocess.run(filter_cmd,
+                                    shell=True,
+                                    stdout=subprocess.PIPE,
+                                    stderr=subprocess.PIPE,
+                                    check=True,
+                                    text=True)
+        except subprocess.CalledProcessError as e:
+            # when the SRC_DIR_PATH is not a git repo
+            if e.returncode == exceptions.GIT_UNINITIALIZED_CODE:
+                init_cmd = f'git -C {expand_src_dir_path} init'
+                subprocess.run(init_cmd,
+                               shell=True,
+                               stdout=subprocess.PIPE,
+                               stderr=subprocess.PIPE)
+                output = subprocess.run(filter_cmd,
+                                        shell=True,
+                                        stdout=subprocess.PIPE,
+                                        stderr=subprocess.PIPE,
+                                        text=True)
+                if git_exclude_exists:
path: sky/data/storage.py
text: @user1:
Instead of checking if git exclude already existed, can we instead check if the .git directory already existed?  If it did, we should leave it untouched (i.e., do not remove any files or directories). This is to avoid inadvertently deleting anything we shouldn't be deleting. 

@author:
If `.git` alreday existed, this `if git_exclude_exists:` statement will not be entered. The outer `if e.returncode == exceptions.GIT_UNINITIALIZED_CODE:` would be entered when `.git` didn't exist in this directory. I'll leave a more detailed comment to note that this exception is caught only when `.git` didn't exist(not a git repo).

```
        except subprocess.CalledProcessError as e:
            # when the SRC_DIR_PATH is not a git repo and .git
            # does not exist in it
            if e.returncode == exceptions.GIT_UNINITIALIZED_CODE:
```

diff_hunk: @@ -2291,6 +2411,85 @@ def test_invalid_names(self, invalid_name_list, store_type):
                 storage_obj = storage_lib.Storage(name=name)
                 storage_obj.add_store(store_type)
 
+    @pytest.mark.parametrize(
+        'gitignore_structure, store_type',
+        [(GITIGNORE_SYNC_TEST_DIR_STRUCTURE, storage_lib.StoreType.S3),
+         (GITIGNORE_SYNC_TEST_DIR_STRUCTURE, storage_lib.StoreType.GCS),
+         pytest.param(GITIGNORE_SYNC_TEST_DIR_STRUCTURE,
+                      storage_lib.StoreType.R2,
+                      marks=pytest.mark.cloudflare)])
+    def test_excluded_file_cloud_storage_upload_copy(self, gitignore_structure,
+                                                     store_type):
+        # tests if files included in .gitignore and .git/info/exclude are
+        # excluded from being transferred to Storage
+
+        with tempfile.TemporaryDirectory() as tmpdir:
+            # Create files to be uploaded in the Storage
+            self.create_dir_structure(tmpdir, gitignore_structure)
+
+            # Create files to be excluded from .gitignore and list them in the file
+            skypilot_path = os.path.abspath(sys.argv[0])
+            skypilot_path = os.path.dirname(skypilot_path)
path: tests/test_smoke.py
text: @user1:
Can we get the path to skypilot repo using `a_module.__file__` instead?  Makes the test robust to invocations from outside of skypilot repo root.

diff_hunk: @@ -2291,6 +2411,85 @@ def test_invalid_names(self, invalid_name_list, store_type):
                 storage_obj = storage_lib.Storage(name=name)
                 storage_obj.add_store(store_type)
 
+    @pytest.mark.parametrize(
+        'gitignore_structure, store_type',
+        [(GITIGNORE_SYNC_TEST_DIR_STRUCTURE, storage_lib.StoreType.S3),
+         (GITIGNORE_SYNC_TEST_DIR_STRUCTURE, storage_lib.StoreType.GCS),
+         pytest.param(GITIGNORE_SYNC_TEST_DIR_STRUCTURE,
+                      storage_lib.StoreType.R2,
+                      marks=pytest.mark.cloudflare)])
+    def test_excluded_file_cloud_storage_upload_copy(self, gitignore_structure,
+                                                     store_type):
+        # tests if files included in .gitignore and .git/info/exclude are
+        # excluded from being transferred to Storage
+
+        with tempfile.TemporaryDirectory() as tmpdir:
+            # Create files to be uploaded in the Storage
+            self.create_dir_structure(tmpdir, gitignore_structure)
+
+            # Create files to be excluded from .gitignore and list them in the file
+            skypilot_path = os.path.abspath(sys.argv[0])
+            skypilot_path = os.path.dirname(skypilot_path)
+            temp_path = f'{tmpdir}/.gitignore'
+            file_path = os.path.join(skypilot_path, 'tests/gitignore_test')
+            shutil.copyfile(file_path, temp_path)
+
+            # Create files to be excluded from .git/info/exclude and list them in the file
+            temp_path = f'{tmpdir}/.git/info/'
+            os.makedirs(temp_path)
+            temp_exclude_path = os.path.join(temp_path, 'exclude')
+            file_path = os.path.join(skypilot_path,
+                                     'tests/git_info_exclude_test')
+            shutil.copyfile(file_path, temp_exclude_path)
+
+            # Create sky Storage with the files created
+            timestamp = str(time.time()).replace('.', '')
+            bucket_name = f'sky-test-{timestamp}'
+            store_obj = storage_lib.Storage(name=bucket_name,
+                                            source=tmpdir,
+                                            mode=storage_lib.StorageMode.COPY)
+            store_obj.add_store(store_type)
+
+        upload_file_name = 'included'
+        # Count the number of files with the given file name
+        up_cmd = self.cli_count_name_in_bucket(store_type, \
+            bucket_name, file_name=upload_file_name)
+        gitignore_cmd = self.cli_count_name_in_bucket(store_type, \
+            bucket_name, file_name='.gitignore')
+        git_exclude_cmd = self.cli_count_name_in_bucket(store_type, \
+            bucket_name, file_name='.git')
+        cnt_num_file_cmd = self.cli_count_file_in_bucket(
+            store_type, bucket_name)
+
+        up_output = subprocess.check_output(up_cmd, shell=True)
+        gitignore_output = subprocess.check_output(gitignore_cmd, shell=True)
+        git_exclude_output = subprocess.check_output(git_exclude_cmd,
+                                                     shell=True)
+        cnt_output = subprocess.check_output(cnt_num_file_cmd, shell=True)
+
+        if store_type == storage_lib.StoreType.GCS:
+            up_dir_cmd = up_cmd = self.cli_count_name_in_bucket(store_type, \
+                bucket_name, file_name=upload_file_name, suffix='include_dir')
+            up_dir_output = subprocess.check_output(up_dir_cmd, shell=True)
+
+        # GCS ls and aws s3 list-objects outputs in a different way
+        # Only 'included.*' files should exist in the cloud object storage
+        if store_type == storage_lib.StoreType.GCS:
path: tests/test_smoke.py
text: @user1:
Can these be in the same if condition block?

diff_hunk: @@ -2291,6 +2411,85 @@ def test_invalid_names(self, invalid_name_list, store_type):
                 storage_obj = storage_lib.Storage(name=name)
                 storage_obj.add_store(store_type)
 
+    @pytest.mark.parametrize(
+        'gitignore_structure, store_type',
+        [(GITIGNORE_SYNC_TEST_DIR_STRUCTURE, storage_lib.StoreType.S3),
+         (GITIGNORE_SYNC_TEST_DIR_STRUCTURE, storage_lib.StoreType.GCS),
+         pytest.param(GITIGNORE_SYNC_TEST_DIR_STRUCTURE,
+                      storage_lib.StoreType.R2,
+                      marks=pytest.mark.cloudflare)])
+    def test_excluded_file_cloud_storage_upload_copy(self, gitignore_structure,
+                                                     store_type):
+        # tests if files included in .gitignore and .git/info/exclude are
+        # excluded from being transferred to Storage
+
+        with tempfile.TemporaryDirectory() as tmpdir:
+            # Create files to be uploaded in the Storage
+            self.create_dir_structure(tmpdir, gitignore_structure)
+
+            # Create files to be excluded from .gitignore and list them in the file
+            skypilot_path = os.path.abspath(sys.argv[0])
+            skypilot_path = os.path.dirname(skypilot_path)
+            temp_path = f'{tmpdir}/.gitignore'
+            file_path = os.path.join(skypilot_path, 'tests/gitignore_test')
+            shutil.copyfile(file_path, temp_path)
+
+            # Create files to be excluded from .git/info/exclude and list them in the file
+            temp_path = f'{tmpdir}/.git/info/'
+            os.makedirs(temp_path)
+            temp_exclude_path = os.path.join(temp_path, 'exclude')
+            file_path = os.path.join(skypilot_path,
+                                     'tests/git_info_exclude_test')
+            shutil.copyfile(file_path, temp_exclude_path)
+
+            # Create sky Storage with the files created
+            timestamp = str(time.time()).replace('.', '')
+            bucket_name = f'sky-test-{timestamp}'
+            store_obj = storage_lib.Storage(name=bucket_name,
path: tests/test_smoke.py
text: @user1:
Can we make this storage object come from a fixture instead of being initialized here? A fixture that uses `yield from self.yield_storage_object`, e.g., `tmp_local_storage_obj`? Otherwise this storage object is not deleted after the test completes.

diff_hunk: @@ -2291,6 +2411,85 @@ def test_invalid_names(self, invalid_name_list, store_type):
                 storage_obj = storage_lib.Storage(name=name)
                 storage_obj.add_store(store_type)
 
+    @pytest.mark.parametrize(
+        'gitignore_structure, store_type',
+        [(GITIGNORE_SYNC_TEST_DIR_STRUCTURE, storage_lib.StoreType.S3),
+         (GITIGNORE_SYNC_TEST_DIR_STRUCTURE, storage_lib.StoreType.GCS),
+         pytest.param(GITIGNORE_SYNC_TEST_DIR_STRUCTURE,
+                      storage_lib.StoreType.R2,
+                      marks=pytest.mark.cloudflare)])
+    def test_excluded_file_cloud_storage_upload_copy(self, gitignore_structure,
+                                                     store_type):
+        # tests if files included in .gitignore and .git/info/exclude are
+        # excluded from being transferred to Storage
+
+        with tempfile.TemporaryDirectory() as tmpdir:
+            # Create files to be uploaded in the Storage
+            self.create_dir_structure(tmpdir, gitignore_structure)
+
+            # Create files to be excluded from .gitignore and list them in the file
+            skypilot_path = os.path.abspath(sys.argv[0])
+            skypilot_path = os.path.dirname(skypilot_path)
+            temp_path = f'{tmpdir}/.gitignore'
+            file_path = os.path.join(skypilot_path, 'tests/gitignore_test')
+            shutil.copyfile(file_path, temp_path)
+
+            # Create files to be excluded from .git/info/exclude and list them in the file
+            temp_path = f'{tmpdir}/.git/info/'
+            os.makedirs(temp_path)
+            temp_exclude_path = os.path.join(temp_path, 'exclude')
+            file_path = os.path.join(skypilot_path,
+                                     'tests/git_info_exclude_test')
+            shutil.copyfile(file_path, temp_exclude_path)
+
+            # Create sky Storage with the files created
+            timestamp = str(time.time()).replace('.', '')
+            bucket_name = f'sky-test-{timestamp}'
+            store_obj = storage_lib.Storage(name=bucket_name,
+                                            source=tmpdir,
+                                            mode=storage_lib.StorageMode.COPY)
+            store_obj.add_store(store_type)
+
+        upload_file_name = 'included'
+        # Count the number of files with the given file name
+        up_cmd = self.cli_count_name_in_bucket(store_type, \
+            bucket_name, file_name=upload_file_name)
+        gitignore_cmd = self.cli_count_name_in_bucket(store_type, \
+            bucket_name, file_name='.gitignore')
+        git_exclude_cmd = self.cli_count_name_in_bucket(store_type, \
+            bucket_name, file_name='.git')
+        cnt_num_file_cmd = self.cli_count_file_in_bucket(
+            store_type, bucket_name)
+
+        up_output = subprocess.check_output(up_cmd, shell=True)
+        gitignore_output = subprocess.check_output(gitignore_cmd, shell=True)
+        git_exclude_output = subprocess.check_output(git_exclude_cmd,
+                                                     shell=True)
+        cnt_output = subprocess.check_output(cnt_num_file_cmd, shell=True)
+
+        if store_type == storage_lib.StoreType.GCS:
+            up_dir_cmd = up_cmd = self.cli_count_name_in_bucket(store_type, \
+                bucket_name, file_name=upload_file_name, suffix='include_dir')
+            up_dir_output = subprocess.check_output(up_dir_cmd, shell=True)
+
+        # GCS ls and aws s3 list-objects outputs in a different way
+        # Only 'included.*' files should exist in the cloud object storage
+        if store_type == storage_lib.StoreType.GCS:
+            assert '2' in up_output.decode('utf-8'), \
+                    f'Filese to be uploaded are not uploaded.: {bucket_name}'
path: tests/test_smoke.py
text: @user1:
Typo

diff_hunk: @@ -873,6 +873,75 @@ def add_if_not_none(key: str, value: Optional[Any]):
         return config
 
 
+def format_gitignore_to_exclude_list(src_dir_path: str) -> List[str]:
+    """Returns a list of formatted excluded files read from .gitignore and
+    .git/info/exclude using git. 'git init' is ran when SRC_DIR_PATH
+    is not a git repository and removed after obtaining excluded list
+    """
+    expand_src_dir_path = os.path.expanduser(src_dir_path)
+
+    git_exclude_path = os.path.join(expand_src_dir_path, '.git/info/exclude')
+    gitignore_path = os.path.join(expand_src_dir_path, '.gitignore')
+
+    git_exclude_exists = os.path.isfile(git_exclude_path)
+    gitignore_exists = os.path.isfile(gitignore_path)
+
+    # This command outputs a list to be excluded according to .gitignore
+    # and .git/info/exclude
+    filter_cmd = f'git -C {expand_src_dir_path} status --ignored -s'
+    excluded_list: List[str] = ['.git/*', '.gitignore']
path: sky/data/storage.py
text: @user1:
We may not want to exclude .gitignore by default, since that is often a part of the repository.

@author:
Cool. Why do we not apply the same logic to `.git/*`?  `.git/*` was excluded by default even before this update.

diff_hunk: @@ -873,6 +873,75 @@ def add_if_not_none(key: str, value: Optional[Any]):
         return config
 
 
+def format_gitignore_to_exclude_list(src_dir_path: str) -> List[str]:
+    """Returns a list of formatted excluded files read from .gitignore and
+    .git/info/exclude using git. 'git init' is ran when SRC_DIR_PATH
+    is not a git repository and removed after obtaining excluded list
+    """
+    expand_src_dir_path = os.path.expanduser(src_dir_path)
+
+    git_exclude_path = os.path.join(expand_src_dir_path, '.git/info/exclude')
+    gitignore_path = os.path.join(expand_src_dir_path, '.gitignore')
+
+    git_exclude_exists = os.path.isfile(git_exclude_path)
+    gitignore_exists = os.path.isfile(gitignore_path)
+
+    # This command outputs a list to be excluded according to .gitignore
+    # and .git/info/exclude
+    filter_cmd = f'git -C {expand_src_dir_path} status --ignored -s'
path: sky/data/storage.py
text: @user1:
Should we use `--porcelain=v1` instead of `-s` here? It promises to stay consistent across versions.

From the docs:
```
--porcelain[=<version>]
           Give the output in an easy-to-parse format for scripts. This is
           similar to the short output, but will remain stable across Git
           versions and regardless of user configuration. See below for
           details.
```

@author:
Nice catch!