Re: [PATCH 7/7] Track common ancestors of a successful conffiledb merge

[Guillem Jover <guillem@debian.org>]

Hi!

On Mon, 2009-12-14 at 08:27:37 +0100, Sean Finney wrote:
> When a merge is successful, store the common ancestor of the merge (the
> pristine version of the conffile from the previous package version) in
> a special location. While this may not have an immediate benefit, it poses
> the oppurtunity for further merge resolution options in future merges.

I don't think that's the only case were we want to keep the ancestor,
this seems to miss the case discussed on the list on one of the previous
threads involving a pristine conffile modified by the admin, then few
pristine versions skipped with --force-confold and later on a merge
happens.

> When the admin later chooses to install the package-shipped version of
> a conffile, or when the package is purged, this information is removed
> from the conffiledb as it is no longer relevant.

> diff --git a/lib/dpkg/test/t-conffiledb.c b/lib/dpkg/test/t-conffiledb.c
> index 8b07e5f..2c32322 100644
> --- a/lib/dpkg/test/t-conffiledb.c
> +++ b/lib/dpkg/test/t-conffiledb.c
> @@ -40,8 +40,8 @@ void ensure_pathname_nonexisting(const char *pathname);
> -char *conffiledbdir;
> +char *conffiledbdir, *mergeparentdir;

This one static too.

> diff --git a/src/conffiledb.c b/src/conffiledb.c
> index baaf7ba..ef56939 100644
> --- a/src/conffiledb.c
> +++ b/src/conffiledb.c
> @@ -43,6 +43,14 @@
> @@ -95,7 +103,7 @@ conffiledb_path(const struct pkginfo *pkg,
>   * conffiledb_path function. It is seperated from the main function
>   * to allow for some internal use where a path needs to be generated
>   * but not from a valid struct versionrevision (i.e. any special "merge"
> - * or "dynamic" versions).
> + * or "dynamic" versions, like #MERGEPARENTDIR).
>   *
>   * For more information on the parameters, see conffiledb_path.
>   *
> @@ -142,7 +150,9 @@ conffiledb_path_cversion(const struct pkginfo *pkg, const char *version,
>   * Ensure that a particular conffiledb dir exists.
>   *
>   * @param pkg The package owning the conffiledb dir.
> - * @param version Which package version to reference.
> + * @param version Which package version to reference. If NULL, the containing
> + *        directory and any other necessary supporting files/dirs are created
> + *        instead (i.e. the merge parent directory).
>   */
>  static void
>  conffiledb_ensure_db(const struct pkginfo *pkg,
> @@ -150,10 +160,19 @@ conffiledb_ensure_db(const struct pkginfo *pkg,
>  {
>  	struct stat s;
>  	int i;
> +	struct varbuf merge_parent_fname = VARBUF_INIT;
>  	char *dbdirs[2];
>  
>  	dbdirs[0] = conffiledb_path(pkg, NULL, NULL);
> -	dbdirs[1] = conffiledb_path(pkg, version, NULL);
> +	if (version) {
> +		dbdirs[1] = conffiledb_path(pkg, version, NULL);
> +	} else {
> +		/* note: the varbuf contents are later free()'d so no need
> +		 *       to varbuffree merge_parent_fname */
> +		varbufprintf(&merge_parent_fname, "%s/%s", dbdirs[0],
> +		             MERGEPARENTDIR);
> +		dbdirs[1] = merge_parent_fname.buf;
> +	}
>  
>  	/* ensure each directory exists while reconstructing the path */
>  	for (i = 0; i < 2; i++) {
> @@ -265,6 +284,80 @@ conffiledb_remove(const struct pkginfo *pkg,
>  }

Abusing those functions to pass something not being a version feels a
bit dirty, and makes them a bit cryptic.

>  /**
> + * Record the common ancestor of a successful merge.
> + *
> + * Assuming pristine v1 conffile was edited locally by the admin, and
> + * then successfully merged with pristine v2 conffile, we then want to
> + * record v1 as a "common ancestor" of the local file and v2, which
> + * may be helpful in future merges.
> + *
> + * @param pkg The package owning the conffile.
> + * @param version The package version of the "common ancestor".
> + * @param path The path to the conffile.
> + */
> +void
> +conffiledb_set_merge_parent(const struct pkginfo *pkg,
> +                            const struct versionrevision *version,
> +                            const char *path)
> +{

The name of the function seems a bit confusing given that it does not
need to be restricted to just merge parents, but just pristine parents
of the currently modified conffile.

> +	char *src_fname, *dst_fname, quoted_filename[256];
> +	struct varbuf tmp_fname_buf = VARBUF_INIT;
> +	int src_fd, tmp_fd;
> +
> +	conffiledb_ensure_db(pkg, NULL);
> +
> +	src_fname = conffiledb_path(pkg, version, path);
> +	dst_fname = conffiledb_path_cversion(pkg, MERGEPARENTDIR, path);
> +	varbufprintf(&tmp_fname_buf, "%s%s", dst_fname, DPKGTEMPEXT);
> +
> +	src_fd = open(src_fname, O_RDONLY);
> +	if (src_fd == -1)
> +		ohshite(_("open '%s' failed"), src_fname);
> +
> +	tmp_fd = open(tmp_fname_buf.buf, O_CREAT|O_TRUNC|O_WRONLY, 0600);
> +	if (tmp_fd == -1)
> +		ohshite(_("open '%s' failed"), tmp_fname_buf.buf);
> +
> +	fd_fd_copy(src_fd, tmp_fd, -1,
> +	           _("recording conffiledb merge ancestor for '%.255s'"),
> +	           path_quote_filename(quoted_filename, src_fname, 256));

Missing fsync here.

> +	if (close(src_fd) == -1)
> +		ohshite(_("close '%s' failed"), src_fname);
> +	if (close(tmp_fd) == -1)
> +		ohshite(_("close '%s' failed"), tmp_fname_buf.buf);
> +
> +	if (rename(tmp_fname_buf.buf, dst_fname) == -1)
> +		ohshite(_("rename '%s' failed"), tmp_fname_buf.buf);
> +
> +	free(src_fname);
> +	free(dst_fname);
> +	varbuffree(&tmp_fname_buf);
> +}

> +void
> +conffiledb_remove_merge_parent(const struct pkginfo *pkg, const char *path)
> +{
> +	char *pkg_dir;
> +
> +	pkg_dir = conffiledb_path_cversion(pkg, MERGEPARENTDIR, path);
> +	ensure_pathname_nonexisting(pkg_dir);

unlink() should be enough in this case.

> +	free(pkg_dir);
> +}

regrads,
guillem