#33 Fix lookaside upload when --path is specified
Merged 8 years ago by lsedlar. Opened 8 years ago by araszka.
araszka/rpkg lookasideFix  into  master

file modified
-6
@@ -2225,9 +2225,6 @@ 

          Can optionally replace the existing tracked sources

          """

  

-         oldpath = os.getcwd()

-         os.chdir(self.path)

- 

          sourcesf = SourcesFile(self.sources_filename, self.source_entry_type,

                                 replace=replace)

          gitignore = GitIgnore(os.path.join(self.path, '.gitignore'))
@@ -2262,9 +2259,6 @@ 

  

          self.repo.index.add(['sources', '.gitignore'])

  

-         # Change back to original working dir:

-         os.chdir(oldpath)

- 

      def prep(self, arch=None, builddir=None):

          """Run rpm -bp on a module

  

no initial comment

I'm not sure if this is the right fix. It works and definitely makes the command from original bugreport work, but I would question whether such usage is actually correct.

The help says Define the directory to work in (defaults to cwd) about --path, so when running with that option, I would expect relative paths to be resolved against that directory. To support this, the operation adds an entry to .gitignore, which in the proposed workflow is weird, because the file was never in there and so there is no need to ignore it.

Alternative is to modify the check in cli.py in new_sources to look for files where --path says and if not found, print an accurate error message.

To support this, the operation adds an entry to .gitignore, which in the proposed workflow is weird, because the file was never in there and so there is no need to ignore it.

No. The file should be added to .gitignore even if "was never in there". Look at it this way...

On my machine:

$ fedpkg new-sources /some/path/to/tarball.tar.xz

Then, on your machine, in a clone of the same repo:

$ fedpkg sources

Sure, the tarball was never in the repo in the first place when I uploaded it. But when you download it, then it is inside the repo for you. Therefore, it is entirely right that we always add the tarball to the .gitignore file, and this is entirely unrelated to the issue at hand.

One thing worth remembering is that if the path to the tarball is a full path, then it doesn't matter whether the user specified --path, the full path is used.

So the problem only happens with relative paths.

Let's put ourselves in the shoes of the user for a second. Taking the example from the original bug report, here is what I imagine happened...

$ fedpkg --path somerep<tab> new-sources tarb<tab>

The first tab-completion would complete the somerepo/ path, and the second one would complete the tarball.tar.gz.

To me this clearly indicates the intent of the user: that tarball.tar.gz is relative to the current working directory, not to the path specified as --path.

This is of course true since just before that the user typed:

$ touch tarball.tar.gz

So they clearly expected the path to be relative to the current working directory.

I think this expectation from the user makes a lot of sense, and it matches a tab-completion-heavy workflow.

Therefore, I do think the behaviour should be that the path to the tarball is considered relative to the current working directory.

Do note that it is my opinion of what should happen.

The pyrpkg code seems to have always done the opposite: make the path to the tarball relative to --path, even before I rewrote the code handling the lookaside cache.

So we have a choice between preserving compatibility and matching user expectations.

I tend to prefer the latter, but maybe that's just me. :smile:

I agree with you that this seems like the more sensible thing to do. Right now it does not work at all when --path is specified, so no matter how we fix it, someone's expectation might be broken.

Pull-Request has been merged by lsedlar

8 years ago
Metadata