Why use 'c.remoteForCopy' instead of 'c.remote' to check length in copy operation?

zhiweio · March 17, 2024, 9:20am

rclone is a fantastic project that I both use and study its source code. I have a question regarding its copy implementation.

In checkPartial(), it checks the length of c.remoteForCopy instead of c.remote.

github.com

rclone/rclone/blob/7803b4ed6c3256f25270e3b3024011f8f1e77ca0/fs/operations/copy.go#L101


      
          	remoteForCopy = c.remote
          	if c.ci.Inplace || c.dstFeatures.Move == nil || !c.dstFeatures.PartialUploads || strings.HasSuffix(c.remote, ".rclonelink") {
          		return remoteForCopy, true, nil
          	}
          	if len(c.ci.PartialSuffix) > 16 {
          		return remoteForCopy, true, fmt.Errorf("expecting length of --partial-suffix to be not greater than %d but got %d", 16, len(c.ci.PartialSuffix))
          	}
          	// Avoid making the leaf name longer if it's already lengthy to avoid
          	// trouble with file name length limits.
          	suffix := "." + random.String(8) + c.ci.PartialSuffix
          	base := path.Base(c.remoteForCopy)
          	if len(base) > 100 {
          		remoteForCopy = TruncateString(c.remoteForCopy, len(c.remoteForCopy)-len(suffix)) + suffix
          	} else {
          		remoteForCopy += suffix
          	}
          	return remoteForCopy, false, nil
          }
          
          // Check to see if we have hit max transfer limits
          func (c *copy) checkLimits(ctx context.Context) (err error) {

I noticed that the copy object is initialized with a default empty string for the attribute remoteForCopy. I suspect that remoteForCopy in copy may never be assigned a value. This confusion leads me to wonder when c.remoteForCopy is assigned a value.

github.com

rclone/rclone/blob/7803b4ed6c3256f25270e3b3024011f8f1e77ca0/fs/operations/copy.go#L381-L399


      
          	c := &copy{
          		f:           f,
          		dstFeatures: f.Features(),
          		dst:         dst,
          		remote:      remote,
          		src:         src,
          		ci:          ci,
          		tr:          tr,
          		maxTries:    ci.LowLevelRetries,
          		doUpdate:    dst != nil,
          	}
          	c.hashType, c.hashOption = CommonHash(ctx, f, src.Fs())
          	if c.dst != nil {
          		c.remote = c.dst.Remote()
          	}
          	// Are we using partials?
          	//
          	// If so set the flag and update the name we use for the copy
          	c.remoteForCopy, c.inplace, err = c.checkPartial()

nielash · March 25, 2024, 10:20pm

I think you might be right. Perhaps at 101 and 103 it should be remoteForCopy (assigned at 91) instead of c.remoteForCopy?

ncw · March 25, 2024, 11:07pm

I think you are right.

That got broken when I refactored it to use a utility object...

zhiweio · March 26, 2024, 1:48am

Hi, Nick, thanks for your efforts, hope you can fix it if it is a bug.

ncw · March 26, 2024, 11:25am

@zhiweio want to send a PR to fix it?

zhiweio · March 27, 2024, 1:23pm

I won't be available until next week, so you can handle it.

ncw · March 30, 2024, 5:38pm

I've merged this to master now which means it will be in the latest beta in 15-30 minutes and released in v1.67 and 1.66.1

kevin498 · April 11, 2024, 11:43am

In the rclone copy implementation, the remoteForCopy attribute of the copy object is initialized with the value of c.remote during the checkPartial() function call. The initialization of the copy object happens in the copyFile() function where c.remote is set to the source remote location (remote) specified for the copy operation. Later, when checkPartial() is called, remoteForCopy is updated based on certain conditions related to partial uploads and filename length considerations. Therefore, remoteForCopy does get assigned a value based on the logic inside checkPartial(), which modifies it according to the specific copy operation settings and constraints.