Re: [PATCH v7 1/3] Introduce bulk-move detection in diffcore.

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 25, 2010, 08:08 UTC
Message-ID: <7veibeitip.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1287868022-24872-2-git-send-email-ydirson@altern.org>

Yann Dirson <ydirson@altern.org> writes:

> The output of raw diff is displayed as "Rnnn a/* b/".  Those cannot be
> confused with renames of files named "whatever/*" with a literal star
> character, from the full-zero SHA1's.

I do not particularly like this asterisk here. It is ambiguous with the case where you renamed a directory whose name is '*' below a/ to a new directory whose name is 'b', isn't it? Using 0{40} as a differentiator like you did is probably a good idea, but then I do not see a sound reason you would need nor want that asterisk.

Show 10 quoted lines

> +Bulk move of all files of a directory into a different one can get
> +detected using the `\--detect-bulk-moves` option.  This adds an
> +additional pass on top of the results of per-file rename detection.
> +They are reported with NULL SHA1 id, in addition to the file renames:
> +
> +------------------------------------------------
> +:040000 040000 0000000... 0000000... R100	a/*	b/
> +:100644 100644 0123456... 1234567... R90	a/file0	b/file3
> +:100644 100644 0123456... 1234567... R100	a/file1	b/file1
> +:100644 100644 0123456... 1234567... R100	a/file2	b/file2

This is kind of interesting. Let's address two issues that should be uncontroversial:

 (1) please do not use a/ and b/, as the reader can easily be confused and
     mistakenly think you are referring to the pre-/post- indicator we use
     when writing textual diffs (e.g. "diff --git a/frotz b/frotz").  You
     are illustrating rename between a directory to another directory,
     both of which are tracked paths.

 (2) The last two lines are R100 but with different object names between
     pre- and post-images, which is wrong and unnecessarily puzzling.
     Please make them match.

These obvious two complaints behind us, there is one more interesting thing in the above, which is _not_ a complaint.

What about renaming of a/file0 to b/file3? Is this part of "all files from directory A moved to directory B"? IOW, is the goal of this series to use the "A/* -> B/" to label the change as bulk directory rename, if the preimage has A/{1,2,3} and the postimage has their moved contents in B/{one,two,three}?

I am wondering about the utility of such an extra information. If there were no "a/file0 -> b/file3" entry in the example, I would imagine that we could use this "a/* -> b/" information to move "a/file5" to "b/file5" when rebasing this patch to apply to a different preimage that had files other than file{1,2} in directory "a", and I would further imagine that might be a wonderful thing.

But if the new "a/* -> b/" hint only gives "things from A/ have migrated to B/ but I can give no information on what name they took under their new home", that may probably reduce the utility of this feature.

I dunno.