Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI: cannot forcibly merge path from UI if merge option not already set #183

Open
g-raud opened this issue Mar 21, 2018 · 4 comments
Open
Labels
discuss way forward is unclear; needs discussion of approach to take and why effort-high issue is likely to require >20h of effort, perhaps much more enhancement issue is a request for a feature, and not a defect

Comments

@g-raud
Copy link
Contributor

g-raud commented Mar 21, 2018

This is about two indepependent but related issues:

  1. The UI lets extra paths be selected to be merged that were not already
    selected by default but fails to merge them.
  2. The error detection and message about that ("'merge' preference not set
    for 'path'" which could be changed to a simple warning, or "'merge'
    preference does not provide a command"), is issued at the time of the
    propagation.

The problem with 2. is that it would be better to fail to select the path for
merging during the interactive selection. Do you agree? I can implement this
for Uitext and maybe for Uigtk2 but not for Uimacbridge; I would keep the
present behaviour when the OS is Mac OSX.

Regarding 1. I would like to be able to select a merge interactively but not
have merge selected by default. The root of the issue is that the preference
merge is overloaded and used for 3 different things: set a merge command
(for a set of paths); decide whether a path is allowed to be merged or not;
and decide whether the default conflict resolution method is by merging.

For backward compatibility "merge command" must remain the associated string
of the preference merge. To solve this properly, by keeping the present
behaviour, allowing users to do what they please and having a clear semantics
for each preference, there are 2 solutions depending on what merge means:

  1. As the set of paths allowed to be merged must have a merge command for the
    merge to be possible, it is natural to use the same preference for both
    "merge command" and "allow to merge", in which case simply adding a
    pathspec preference mergedeselect to not "merge by default" is sufficient.
  2. Taking merge to mean "merge by default" would require to be able to set
    associated strings without setting the preference, which is already
    implemented by assoc patterns (PR Assoc only Pathspec Patterns #178). No new preference is strictly
    necessary as the paths allowed to be merged could simply be all those that
    have a "merge command"; the code checking that merge is set to allow to
    merge would have to be removed though, which is backward-compatible because
    at present merge is always set when a merge command is defined (in the
    version before the addition of del/assoc patterns to be precise).

The solution 2. is more convenient because it allows to set a merge command
without selecting merge as the default conflict resolution in a single profile
setting when solution 1. requires two settings:

merge = assoc Path pathspec -> mergecmd CURRENT1 CURRENT2

These solutions can be refined by adding options to warn when selecting a non
default merge, and reduce the set of "allow to merge" to be smaller than
"merge command is set" (which is useful when one defines very generic merge
commands in a file included by many profiles):

  1. Convert the associated string of mergedeselect to a 3-valued type:
    "allow", "warn" or "disable" (default: "allow"), and use assoc only
    patterns to set generic merge commands.
  2. Add a client-local pathspec preference mergedisable and optionally
    convert its associated string to a 2-valued type "warn", "disable" (plus an
    optional "allow" that would be like not setting mergedisable) (default:
    "disable").

Would solution 2. be accepted?

@brabalan
Copy link
Collaborator

I haven't used merge for a long time, so please correct me if I make wrong assumptions.

First, I'm surprised that we can change the resolution of two files to a merge, as it could not succeed. The only case I can think of where it would make sense (there is a merge command associated to the path) is when only one replica has changed, and then merging does not make sense. As a first reaction, I would suggest to disable the ability to set the resolution as a merge manually. I'm fine with having an immediate error when trying to select such a resolution (as it would be useful with the rest of your proposal).

However, I agree that being able to separately specify how to merge files and which files to merge is quite useful. The mergedeselect may be the simplest to implement, but it's starting to pile more preferences on top of preferences. I haven't looked at the assoc approach yet, but it does seem more elegant. I'm not sure about the warning options you propose at the end, as they seem quite complex.

@g-raud
Copy link
Contributor Author

g-raud commented Mar 22, 2018

First, I'm surprised that we can change the resolution of two files to a merge, as it could not succeed. The only case I can think of where it would make sense (there is a merge command associated to the path) is when only one replica has changed, and then merging does not make sense. As a first reaction, I would suggest to disable the ability to set the resolution as a merge manually. I'm fine with having an immediate error when trying to select such a resolution (as it would be useful with the rest of your proposal).

The UI allows to set conflict resolution to merge only for conflicts of course (which can be to simply reset to the default), but if the default was not already to merge then the merge never succeeds because the merge preference is not set.

However, I agree that being able to separately specify how to merge files and which files to merge is quite useful. The mergedeselect may be the simplest to implement, but it's starting to pile more preferences on top of preferences. I haven't looked at the assoc approach yet, but it does seem more elegant. I'm not sure about the warning options you propose at the end, as they seem quite complex.

In fact solution 2. would be slightly easier to implement because assoc patterns are already implemented; both however are easy changes to make. Having the warning happen during the interactive reconciliation could be much harder (I haven't looked at the code of Uigtk2 nor Uimac yet).

@g-raud
Copy link
Contributor Author

g-raud commented Mar 22, 2018

In fact I don't even know what the Mac UI looks like nor if it can do more or less things than the Gtk one. Which makes think it would be nice to have screenshots on the website and/or in the README file on github.

@g-raud
Copy link
Contributor Author

g-raud commented Mar 22, 2018

In fact the UI allows to set conflict resolution to merge even for non conflicting files. I don't think it is coherent with what Unison does. It could nonetheless have its use for example to normalize a file after data has been appended to a it (for example sort a log pertaining to a set of files to have all "events" relating to a same file be contiguous).

So it leads to a new choice: what to do when in the UI a path that has a merge command but that is not conflicting is selected to be merged?

@gdt gdt added effort-high issue is likely to require >20h of effort, perhaps much more enhancement issue is a request for a feature, and not a defect labels Sep 14, 2020
@gdt gdt added the discuss way forward is unclear; needs discussion of approach to take and why label Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss way forward is unclear; needs discussion of approach to take and why effort-high issue is likely to require >20h of effort, perhaps much more enhancement issue is a request for a feature, and not a defect
Projects
None yet
Development

No branches or pull requests

3 participants