Hi,
On Wed, Jul 13, 2011 at 1:17 AM, Johannes Sixt <j.sixt@xxxxxxxxxxxxx> wrote:
> Am 6/8/2011 9:30, schrieb Elijah Newren:
>> +static int make_room_for_path(const struct merge_options *o, const char *path)
>> {
>> - int status;
>> + int status, i;
>> const char *msg = "failed to create path '%s'%s";
>>
>> + /* Unlink any D/F conflict files that are in the way */
>> + for (i = 0; i < o->df_conflict_file_set.nr; i++) {
>> + const char *df_path = o->df_conflict_file_set.items[i].string;
>> + size_t pathlen = strlen(path);
>> + size_t df_pathlen = strlen(df_path);
>> + if (df_pathlen < pathlen && strncmp(path, df_path, df_pathlen) == 0) {
>> + unlink(df_path);
>> + break;
>> + }
>> + }
>
> Each time this loop is entered it tries to remove the same path again,
> even if it does not exist anymore or was morphed into a directory in the
> meantime. I suggest to remove a path from o->df_conflict_file_set after it
> was unlinked. Or even better: have a separate "make room" phase somewhere
> in the merge process.
Removing it from o->df_conflict_file_set makes sense. However, there
appears to be no API in string_list.h for deleting entries. Are such
operations discouraged? I'm not sure whether to add such API, just
hack it directly, or wait for someone else to come along and change
this to a better data structure (such as a hash)...
I don't think it's possible to move this "make room" phase anywhere
earlier in the merge process. When we have D/F conflicts, the files
of those D/F conflicts should only be removed if at least one of the
paths under the corresponding directory are not removed by the merge
process. We don't know whether those paths will need to be removed
until we call process_entry() on each of them, and from there we go
right to this function when we find one that needs to stick around.
So I simply don't see how to move it any earlier.
However, there is clearly a bug in this code. If there is a D/F
conflict at 'd' (e.g. paths 'd' and 'd/foo' are present), and there is
a file named 'd_bla', then the need to merge 'd_bla' can cause 'd' to
be deleted. Oops. (Granted, it's a bug that would be masked by the
later call to process_entry() on 'd', which would reinstate that file
if necessary, but that doesn't mean this code is right.) I'll fix
that up.
However, I don't see how any of this would address any failure you're
seeing on windows. Maybe one of my other changes, including one or
two other bugfixes I've found will help? I'll have to ping you when I
submit the re-roll.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html