Re: [PATCH v3 02/11] merge-one-file: rewrite in C

Alban Gruin [off-list ref] writes:

quoted

quoted

+	/*
+	 * Create the working tree file, using "our tree" version from
+	 * the index, and then store the result of the merge.
+	 */

The above is copied from the original, to explain what it did after
the comment, but it does not seem to match what the new code does.

quoted

+	ce = index_file_exists(istate, path, strlen(path), 0);
+	if (!ce)
+		BUG("file is not present in the cache?");
+
+	unlink(path);
+	if ((dest = open(path, O_WRONLY | O_CREAT, ce->ce_mode)) < 0) {
+		free(result.ptr);
+		return error_errno(_("failed to open file '%s'"), path);
+	}
+
+	written = write_in_full(dest, result.ptr, result.size);
+	close(dest);
+
+	free(result.ptr);
+
+	if (written < 0)
+		return error_errno(_("failed to write to '%s'"), path);
+

This open(..., ce->ce_mode) call is way insufficient.

The comment we have above this part of the code talks about the
difficulty of doing this correctly in scripted version.  Creating a
file by 'git checkout-index -f --stage=2 -- "$4"' and reusing it to
store the merged contents was the cleanest and easiest way without
having direct access to adjust_shared_perm() to create a working
tree file with the correct permission bits.

The original that the comment applies to does this

	git checkout-index -f --stage=2 -- "$4" && cat "$src1" >"$4" || exit 1

to create path "$4" with the correct mode bits, instead of a naïve

	mv "$src1" "$4"

because the filemode 'git checkout-index -f --stage=2 -- "$4"' gives
to file "$4" is by definition the most correct one for the path.
The command knows how user's umask and type bits in the index should
interact and produce the final mode bits, but "$src1" was created
without any regard to the mode bits---the 'git merge-file' command
only cares about the contents and not filemode.  We can even lose
the executable bit that way.  And preparing "$4" and then catting
the computed contents into it was a roundabout way (it wastes the
entire writing-out of the contents from the index), and that was
what the comment was about.

But all that is unnecessary once you port this to C.  So the comment
does not apply to the code you wrote, I think, and should just be
dropped.