set directory permissions with bulk()

[nfriedly]

Hi, I have some code like this:

  zip.bulk([
    { expand: true, cwd: sourceDir,  src: ['**', '**/.htaccess'], data: { mode: 0644 }  }
  ]);

And I've noticed that when creating a .zip, the mode is applied to files but not directories. (The directory permissions on the disk are 755, but the resulting .zip has 700.)

I'm working on a mac, and I believe I've seen similar behavior on linux, so I'm not sure if this is related to the other open bugs or not. Either way, if there is a feature that lets me set directory permissions when doing a bulk operation that would be ideal, because I know what output permissions I want regardless of what they are on disk.

(I glanced at the new .directory() method, but I didn't see anything different there. And I'm guessing that it would drop the .htaccess file, so it probably wouldn't work for me anyway.)

Any thoughts? Should I just add a directoryData option and send a PR?

[ctalkington]

its true that bulk and directory really don't account for mode on directories. theres a few issues with the way mode is handled on directories (which is a newer addition to archiver) but those appear to mainly be on windows due to stat returning a 06xx mode.

ive been considering letting those data props also be functions that receive file data and return modified data as I really dont want to add another prop or argument.

also, if ever in question of what is really used internally, the best way is to switch to json format which will output all the files with whole stat object and file data.

the next version will most likely address mode issues once i finish the plugin overhaul.

[nfriedly]

Ok, that's cool. Letting data be a function would work for me. Want me to send a PR to do that?

[ctalkington]

have a peak at 6688585

[nfriedly]

I left one comment on the commit, but that looks like it will work for me. Thanks for the quick turnaround!

[ctalkington]

im not sure theres much case for logging in that instance, as by default itd fail since false isnt a func, then data would need to be a function to even be used by the code. the || bit is just so that if someone doesnt return it doesn't break things.

id honestly opt for breaking before implementing logging for that case.

[ctalkington]

at the end of the day, using a function is an advanced use case, as such its NOT going to normalize or verify what you give it. you're expected to know what your doing when using it.

[nfriedly]

Right, the issue isn't with the default false, it's if I as a user do something stupid like

  zip.bulk([
    { expand: true, cwd: sourceDir,  src: ['**'], data: function() { throw 'foo';}  }
  ]);

It will appear to work successfully because the foo error gets swallowed. I think that error should be thrown, or if not that, then at least provide some way for me to see what happened.

If the error is less obvious (e.g. calling some other lib that throws in some circumstances), that's going to be a nightmare to debug.

[ctalkington]

this should cover most cases without being too insane with checking.

c0f3b86

[ctalkington]

fyi if you need this right now, archiver@0.15.0 should now work as a semver match since 0.15.0-1 (alpha) is now published.

[nfriedly]

that works, thanks!

[ctalkington]

glad to hear, i usually don't publish for every change so alphas should still be mostly safe if not its good to hear about issues before final goes out.

[nfriedly]

Yep. BTW, in case anyone else wants it, this is the function I'm using:

function setData(data) {
  data.stats || (data.stats = fs.statSync(path.join(sourceDir, data.name)));
  if (data.stats.isDirectory()) {
    data.mode = 0755; // octal
  } else {
    data.mode = 0644;
  }
  return data;
};

Doing a synchronous stat isn't ideal, but this code doesn't get run very often and (I think) it will at least ensure that the file doesn't get stat'd twice.

[ctalkington]

yah, that should work ok since theres logic to check if stats are already set in _append.

i think bulk is the only one that wouldn't have stats at that point. that is likely to become mute whenever src(glob) is implemented. bulk has always been a hack to allow for some more advanced cases.

[nfriedly]

Cool. Thanks again!

[ctalkington]

also, did you find out if the directory method works in your use case? I believe it should get .htaccess as its just reading dir and pulling everything.

[ctalkington]

the code for that should be:

zip.directory(sourceDir, false, setData);

[nfriedly]

Yea, I just tried it out, and it looks like it does work. It also grabs the .gitignore, which the bulk method skipped, but that's not a big deal either way.

[ctalkington]

yah, this is where src will fill the void as itll support negative globs. ideally bulk will be removed a few versions out as its not the best for async usage either.

[arichiardi]

So I have a question on this as I have the same requirement as the OP, having to add dirs recursively and set the mode to 755 for them.

Can you confirm that this would not work recursively?

zip.directory(sourceDir, {mode: 755})