Re: [PATCH 5/6] Implement 'm' option for conffile merging during conflict resolution

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

A few quick thoughts and questions about conffiledb_automerge:

Sean Finney wrote:

> This functions by performing a 3-way diff using the new conffile, the
> currently-installed conffile, and the pristine version of the conffile
> shipped in the currently installed package (in the conffile database).
[...]
 
> +int conffiledb_automerge(const char *pkg, const char *path)
> +{

This merges a user’s and package’s changes to update a conffile, also
saving the result to the resolved conffile db.

> +	/* i.e. <path> + ".dpkg-merge" + '\0' */
> +	merge_output_fname_sz = strlen(path) + strlen(DPKGMERGEEXT) + 1;
> +	merge_output_fname = m_malloc(merge_output_fname_sz);
> +	sprintf(merge_output_fname, "%s%s", path, DPKGMERGEEXT);

Why save here rather than a tmp conffile db?  Or, put another way:
why do we write to the conffile itself before the conffile db?

> +	/* create a file for merge output, ensuring it does not already exist */
> +	merge_fd = open(merge_output_fname, O_CREAT|O_WRONLY|O_EXCL);
> +	if (merge_fd == -1 )
> +		ohshite("conffiledb_automerge: can't open %s for merge output",
> +		        merge_output_fname);

If dpkg fails part-way through, the .dpkg-merge will be left lying
around.  Would it be safe to delete it and try again in the next run?

> +	if (WEXITSTATUS(res) == 0) {
> +		/* rename the merge output to the final destination */
> +		if (rename(merge_output_fname, path) == -1)
> +			ohshite("conffiledb_automerge: can not rename %s", 
> +			        merge_output_fname);
> +		/* and the also register the successful merge in the
> +		 * "resolved" conffiles db, as another possible ancestor for
> +		 * future merges */
> +		path_fd = open(path, O_RDONLY);
> +		if (path_fd < 0)
> +			ohshite("conffiledb_automerge: can not open %s", path);
> +		conffiledb_ensure_db(pkg, conffiledb_base_resolved);
> +		cf_resolved = conffiledb_path(pkg, path, 
> +		                              conffiledb_base_resolved);
> +		resolved_fd = open(cf_resolved, O_WRONLY|O_CREAT|O_TRUNC);
> +		if (resolved_fd < 0)
> +			ohshite("conffiledb_automerge: can not open %s", 
> +			        cf_resolved);
> +		fd_fd_copy(path_fd, resolved_fd, -1, "conffiledb_automerge");

This could be made very similar to the conffile unpacking code if dpkg
writes to the tmp db first.  If diff3 succeeds, dpkg could copy to the
.dpkg-merge file, then rename both.

I see you save the resolved version in the db for future reference,
so we can tell later if the user decides to modify the result.  Most
version control systems do something like this to cache the result and
avoid reading the working tree before commiting it to the repository.
Is dpkg also preparing to use the file later?

If the resolved version will be the "common ancestor" in future
merges, what files will be "mine" and "yours"?  I would not imagine
future distributed conffiles could play either role, since the the
resolved version contains both good (user-written) and bad
(distributor-corrected) changes relative to them.

Jonathan