Merge ~adrien/git-ubuntu:worktree-compatibility into git-ubuntu:main

[Adrien Nader]

Make "prepare-upload args" usable in git worktrees

First, this fixes a codepath that was attempting to create a new repository and only worked in
non-worktrees by chance.

Second, this checks for the absence of ident, text and eol attributes in a generic way that also
works in worktrees. It cannot add the relevant lines in the attributes configuration for
worktrees but at least the code can spot if they are already present.

[Adrien Nader (adrien)]

The trigger for these changes was me getting upload rights but prepare-upload failing in my worktree-based setup.

[Adrien Nader (adrien)]

Sorry, I got distracted and made a mistake somewhere. I'll drop the Needs review status and change it again once I've fixed it.

I would appreciate if you could take a high-level look and comment on the overall approach (especially with the first commit).

Thanks!

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:1b3a2ccecb8d619c4d19588357467d076ff67b6d

No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want jenkins to rebuild you need to trigger it yourself):
https://code.launchpad.net/~adrien/git-ubuntu/+git/git-ubuntu/+merge/483227/+edit-commit-message

https://jenkins.canonical.com/server-team/job/git-ubuntu-ci/63/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: VM Reset
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/git-ubuntu-ci/63//rebuild

review: Needs Fixing (continuous-integration)

[Adrien Nader (adrien)]

I've fixed the code and added a message for the merge commit.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:81f89cde74fd1b8ffc44fe8b294b4e0b2f9bf19a
https://jenkins.canonical.com/server-team/job/git-ubuntu-ci/64/
Executed test runs:
    SUCCESS: VM Setup
    FAILED: Build

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/git-ubuntu-ci/64//rebuild

review: Needs Fixing (continuous-integration)

[Robie Basak (racb)]

Thank you for working on this!

Please could you add some tests?

This code is very old and some of it predates tests, but I think the changes you're making are quite testable and it shouldn't be burdensome to add some.

You could ensure that a working GitUbuntuRepository is instantiated in the cases of a) directory doesn't exist; b) directory exists but is empty; c) directory exists and is a git repository, and further test round tripping of data if a GitUbuntuRepository is re-instantiated.

As you're fixing the specific case of worktree compatibility, you should definitely do the above with and without worktrees.

See the existing tests for details on how to do "parametrization" and in use of the pytest tmpdir fixture.

For the attributes file, I'm not sure of the exact semantics required, but seems like it would a good idea to break out a "ensure attributes file is set as required" function, as it looks like that's what you're reimplementing here. Then you could write unit tests for various starting states to ensure that the attributes file is correctly created just as it was before.

You're probably already doing this but please also get CI passing :)

review: Needs Fixing

Unmerged commits

81f89cd... by Adrien Nader on 2025-03-21

Simplify set_git_attributes and make it compatible with git worktrees

With git worktrees, .git is a file that is merely a link to the actual
storage of git data. This includes configuration and attributes.

The logic in git-ubuntu requires that .git/info/attributes could be opened
to add attributes configuration. It also had lengthy and repetitive code
in case the file already existed and maybe already contained the lines
that were wanted.

For git worktrees, the correct file is located in the main repository for
the worktrees (I guess this implies there is no per-worktree attributes,
although there is per-worktree configuration). Fortunately, there is some
support for worktrees in libgit2/pygit2 and we can query attributes
directly instead of spawning grep or parsing ourselves a file in a variable
location.

The code logic can also be turned around and written with idempotence in
mind. For each attribute, we query attributes for a non-existing file
(since we're looking for an attribute that matches '*'), record if the
attributes don't match our expectations, and finally add all attributes
that may be needed.

This does not support adding attributes in worktrees but once the user has
done it, the code will not attempt to do it again which is an improvement.

aeb48ec... by Adrien Nader on 2025-03-21

Try to open the given dir as a repository before initializing one

GitUbuntuRepository has always used pygit2.init_repository() which can
actually open a repository that exists. This has made it possible to use
the code both to create a repository and to use an existing repository.

However, init_repository is not intended to open existing repositories: it
only works by chance and actually fails on worktrees.

This commit will first try to open the given path as a repository, and if
it fails, it will try to init a repository. This matches the semantics of
libgit2/pygit2. However, due to the organically-grown dual semantics of
GitUbuntuRepository exposed above, regressions can't be ruled out.

diff --git a/gitubuntu/git_repository.py b/gitubuntu/git_repository.py
index e57dc1d..1160edc 100644
--- a/gitubuntu/git_repository.py
+++ b/gitubuntu/git_repository.py
@@ -1110,7 +1110,10 @@ class GitUbuntuRepository:
                     sys.exit(1)
             self._local_dir = local_dir
 
-        self.raw_repo = pygit2.init_repository(self._local_dir)
+        try:
+            self.raw_repo = pygit2.Repository(self._local_dir)
+        except pygit2.GitError:
+            self.raw_repo = pygit2.init_repository(self._local_dir)
         # We rely on raw_repo.workdir to be identical to self._local_dir to
         # avoid changing previous behaviour in the setting of GIT_WORK_TREE, so
         # assert that it is so. This may not be the case if the git repository
@@ -1410,61 +1413,27 @@ class GitUbuntuRepository:
         git_attr_path = os.path.join(self.raw_repo.path,
                                      'info',
                                      'attributes'
-                                    )
-        try:
-            # common-case: create an attributes file
-            with open(git_attr_path, 'x') as f:
-                f.write('* -ident\n')
-                f.write('* -text\n')
-                f.write('* -eol\n')
-        except FileExistsError:
-            # next-most common-case: attributes file already exists and
-            # contains our desired value
-            try:
-                runq(['grep', '-q', '* -ident', git_attr_path])
-            except CalledProcessError:
-                # least-common case: attributes file exists, but does
-                # not contain our desired value
-                try:
-                    with open(git_attr_path, 'a') as f:
-                        f.write('* -ident\n')
-                except:
-                    # failed all three cases to set our desired value in
-                    # attributes file
-                    logging.exception('Unable to set \'* -ident\' in %s' %
-                                      git_attr_path
-                                     )
-                    sys.exit(1)
-            try:
-                runq(['grep', '-q', '* -text', git_attr_path])
-            except CalledProcessError:
-                # least-common case: attributes file exists, but does
-                # not contain our desired value
-                try:
-                    with open(git_attr_path, 'a') as f:
-                        f.write('* -text\n')
-                except:
-                    # failed all three cases to set our desired value in
-                    # attributes file
-                    logging.exception('Unable to set \'* -text\' in %s' %
-                                      git_attr_path
                                      )
-                    sys.exit(1)
+        # This is a random (256 bits of entropy) filename that is expected to
+        # not exist and not be configured. Therefore, the only way to match it
+        # in git attributes is using a '*' selector.
+        enoent_file = '7dqmcHiVCS42r8KbxbJUetr4R2SlynQ2nR0MLwc4qUA='
+
+        missing_attr = []
+        for attr in ['ident', 'text', 'eol']:
+            if self.raw_repo.get_attr(enoent_file, attr) != False:
+                missing_attr.append(attr)
+
+        if missing_attr:
             try:
-                runq(['grep', '-q', '* -eol', git_attr_path])
-            except CalledProcessError:
-                # least-common case: attributes file exists, but does
-                # not contain our desired value
-                try:
-                    with open(git_attr_path, 'a') as f:
-                        f.write('* -eol\n')
-                except:
-                    # failed all three cases to set our desired value in
-                    # attributes file
-                    logging.exception('Unable to set \'* -eol\' in %s' %
-                                      git_attr_path
-                                     )
-                    sys.exit(1)
+                with open(git_attr_path, 'ab') as f:
+                    for attr in missing_attr:
+                        f.write(f'* -{attr}\n')
+            except:
+                logging.exception('Unable to set attributes in %s' %
+                                  git_attr_path
+                                  )
+                sys.exit(1)
 
     def remote_exists(self, remote_name):
         # https://github.com/libgit2/pygit2/issues/671