Re: [PATCH] builtin-apply: keep information about files to be deleted

Michał Kiedrowicz [off-list ref] writes:

Junio C Hamano [off-list ref] wrote:

quoted

Michał Kiedrowicz [off-list ref] writes:
  

quoted

... However, there are some cases when these two
rules may cause problems:

patch #1: rename A to B
patch #2: rename C to A
patch #3: modify A

Should patch #3 modify B (which was A) or A (which was C)?

patch #1: rename A to B
patch #2: rename B to A
patch #3: modify A
patch #4: modify B

Which files should be patched by #3 and #4?

In my opinion both #3 and #4 should fail (or both should succeed) --
with my patch only #3 will work and #4 will be rejected, because in
#2 B was marked as deleted.  

Both of the examples above cannot be emitted as a single commit by
format-patch; the user is feeding a combined patch.  Perhaps renames
in each example sequence were came from one git commit but
modifications are from separate commit or handcrafted "follow-up"
patch.

Yes, that's true. In "normal" case, renames and modifications should be
handled properly and (generally) aren't subject of this discussion.

quoted

There are two stances we can take:

 (1) The user knows what he is doing.

     In the first example, if he wanted the change in #3 to end up in
B, he would have arranged the patches in a different order, namely, 3
1 2, but he didn't.  We should modify A (that came from C).

 (2) In situations like these when it is unusual and there is no
clear and unambiguous answer, the rule has always been "fail and ask
the user to clean up", because silently doing a wrong thing in an
unusual situation that happens only once in a while is far worse than
     interrupting the user and forcing a manual intervention.

     In the first example, there is no clear answer.  Perhaps all
three patches were independent patches (the first two obviously came
from git because only we can do renames, but they may have been
separate commits), and the user may have reordered them (or just
picked a random order because he was linearizing a history with a
merge).

The second one is even iffier.  If we _know_ that originally patch #1
and #2 came from the same commit, then they represent swapping
between A and B, but if they came from different git commits, and if
the user picked patches in a random order, it may mean something
completely different.

The problem here is that there are at least two patches which touch the
same file(s) and it is impossible to say which patches should be handled
atomically. However, there is no easy way to specify renames as a
single patch. A diff containing swapping of three files looks like this:

	diff --git a/file2 b/file1
	similarity index 100%
	rename from file2
	rename to file1
	diff --git a/file3 b/file2
	similarity index 100%
	rename from file3
	rename to file2
	diff --git a/file1 b/file3
	similarity index 100%
	rename from file1
	rename to file3

BTW: it applies correctly :).

quoted

I am somewhat tempted to say that we should fail all of them,
including the original "single patch swapping files" brought up by
Linus.

I may agree that difficult scenarios should be rejected, but I will
also say that git-apply should always accept git-diff output.

quoted

BUT

Can we make use simple rule to detect problematic cases?

 - An input to git-apply can contain more than one patch that affects
a path; however

   - you cannot create a path that still exists, except for a path
that _will_ be renamed away or removed (your patch fixes this by
adding this "except for..." part to loosen the existing rule);

   - you cannot modify a path in a separate patch if it is involved
in an either side of a rename (this will catch the ambiguity of patch
#3 in your first example and #3 and #4 in your second example);

What should happen in following situation:

patch #1: modify A
patch #2: rename A to B

#2 should fail? Now it creates new B which is a copy of A before
applying any patches and modifies A according to #1.

Yes.  It is obviously a handcrafted sequence, and it could even have been
mechanically created.

Imagine a merge of two branches like this:

               2----HEAD
              /    /
	common----1

and somebody fed "common..HEAD" to his script that internally runs
format-patch and squashes the patch output into one, perhaps:

	#!/bin/sh
	# Create a single patch e-mail, squashed.
        tmp=/var/tmp/my-squash$$
	rm -rf "$tmp" && mkdir -p "$tmp/out" || exit
        trap 'rm -rf "$tmp"' 0 1 2 3 15
	git format-patch -o "$tmp/out" "$@"
	>"$tmp/all.messages"
        >"$tmp/all.patches"
        for mail in "$tmp"/out/0*
        do
        	git mailinfo "$tmp/msg" "$tmp/patch" >"$tmp/info" <"$mail"
                echo "$mail" >>.messages
                cat "$tmp/msg" >>"$tmp/all.messages"
                cat "$tmp/patch" >>"$tmp/all.patches"
	done
	(
	        cat "$tmp/info"; echo
                cat "$tmp/all.messages"; echo
	        cat "$tmp/all.patches"
	)

Depending on the sort order between #1 and #2, you cannot tell "modify A
and then rename it to B" is the order we will see such a patch at all.  I
think it is safer to reject such a patch with the "when in doubt, do not
act too clever and risk making a silent mistake" principle.

In this particular case, the reverse order of "renaming A to B" and then
"modifing A" would fail anyway, but if you have another patch that renames
C to A in the mix of patches whose order cannot be determined, I think you
can come up with a sequence that results in an "applicable in order but is
the order really what the author intended?" situation.

Do you mean that patches which break above rules should be
skipped when "--reject" is set, as other failures? Or that
whole git-apply should fail regardless of "--reject"?

I meant the latter.