andsens / homeshick

git dotfiles synchronizer written in bash
MIT License
2.1k stars 147 forks source link

overwrite files with same content #204

Closed laur89 closed 2 years ago

laur89 commented 3 years ago
laur89 commented 3 years ago

files_are_same() could be extended to also support logical json comparison via jq:

files_are_same() {
  local f1="$1"
  local f2="$2"

  if jq -en --slurpfile a "$f1" --slurpfile b "$f2" '$a == $b' >/dev/null 2>&1; then
    return 0
  elif command -v cmp >/dev/null 2>/dev/null; then
    cmp --silent -- "$f1" "$f2"
    return $?
  elif command -v diff >/dev/null 2>/dev/null; then
    diff -- "$f1" "$f2" >/dev/null 2>/dev/null
    return $?
  fi

  # if we don't have any supported file comparison tools availabe, return falsey:
  return 1
}

This should probably be opt-in feature though.

@andsens what's your view on this?

andsens commented 3 years ago

Apologies for the late answer. I've had a little think about this. The json comparison is definitely too complex for something like homeshick. By relying on jq you introduce a dependency. All the ways to handle that dependency would add complexity.

  1. Ignore if jq is not installed: Now you have different behavior on different machines
  2. Give an error if jq is not installed: Same as above, with the added annoyance of homeshick suddenly being able to error out
  3. Allow user to configure whether that feature should be disabled or opt for (1) or (2): Now we have added the complexity of configuration, something that homeshick has never needed before.

Regarding the original change: Could you give some concrete scenarios where this is necessary? I know some tools that do what you describe, but that almost always goes hand-in-hand with changes in the file, meaning the feature wouldn't kick in anyways.
I'm just concerned that we are adding 100 extra lines of code to handle something that pretty much never happens and can be dealt with manually when it does.

laur89 commented 2 years ago

No worries, it's not an urgent feature anyways.

The json comparison is definitely too complex for something like homeshick. By relying on jq you introduce a dependency

True, but all the dependencies are handled in a safe way, ie if dependency is not avail or its execution fails, we default back to as-is function. I'd prefer to make this an opt-in boolean config that most users wouldn't have to ever even think about. Only ones taking their time going through the docs might get interested.

Regarding the original change: Could you give some concrete scenarios where this is necessary?

I've noticed Deluge (a torrenting daemon) do that to its config and some of its plugin config files. Have seen it years before (long-time homeshick customer here), but don't recall the details.

It's by no means a needed feature, just a QoL thing. I know homeshick is way more complex of a program than it sounds, so if you feel uncomfortable piling on a feature few people would be interested in, I absolutely understand.

andsens commented 2 years ago

opt-in boolean config

Yeah, and that's the problem. I've managed to keep homeshick config-free so for now. The reason being that config files introduce a lot of complexity and allow for feature creep. Additionally behavior can be altered based on the config, making troubleshooting much harder. It's powerful of course, but after so many years of maintaining it without, I've come to the conclusion that for this piece of software at least, it's simply not worth the trouble. The simplicity is a strength in this case.

Looking at deluge I can see that they actually use realpath() to resolve the configfile path. This was an issue in an older version (see here). Alternatively you can always convert the directory in which the config resides into a symlink, then deluge can do as many overwrites as it likes while you still track the changes.

andsens commented 2 years ago

I'm closing it for now. The use case is valid, but I'd much rather like the individual programs that are misbehaving to get fixed. This way the issues can eventually become resolved, rather than everybody having to work around them.

andsens commented 2 years ago

(Though I might cherry-pick some of your syntax fixes though, also good job on the tests! This was a solid PR!)