Strange output of 'git diff <revision1>:<file> <revision2>:<file>'

Strange output of 'git diff <revision1>:<file> <revision2>:<file>'

[Jakub Narebski]

git diff output for files specified by revision is somewhat unexpected. 

  $ git diff <revision_1>:<file> <revision_2>:<file>

outputs the following diff metainfo

  diff --git a/<revision_2>:<file> b/<revision_2>:<file>
  index 5eabe06..2e87de4 100644
  --- a/<revision_2>:<file>
  +++ b/<revision_2>:<file>

Is it intended, or is it a bug? Looks like a bug to me...

git 1.4.0 (git-core-1.4.0-1.fc4 by Fedora Project).
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

[PATCH] use the path as name from the revision:path syntax in setup_revisions

[Matthias Lederhofer]

---
Jakub Narebski <jnareb@gmail.com> wrote:
> git diff output for files specified by revision is somewhat unexpected. 
> 
>   $ git diff <revision_1>:<file> <revision_2>:<file>
> 
> outputs the following diff metainfo
> 
>   diff --git a/<revision_2>:<file> b/<revision_2>:<file>
>   index 5eabe06..2e87de4 100644
>   --- a/<revision_2>:<file>
>   +++ b/<revision_2>:<file>
> 
> Is it intended, or is it a bug? Looks like a bug to me...
I guess the revision should not be there.  Here is a patch to remove
it.
<matled> is there a name for 'revision:path' to specify a file at a
         certain revision?
<jdl> "That weird syntax, you know."
<matled> get_path_out_of_the_weird_syntax(const char *s)
<matled> you know :)
<jdl> That should do it!
Could someone please come up with a nice name for the function?
Without knowing a name for the '<revision>:<path>' syntax I can only
think of get_path or similar names and this is much too generic imo.
---
 cache.h     |    1 +
 revision.c  |    2 ++
 sha1_name.c |   12 ++++++++++++
 3 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/cache.h b/cache.h
index d433d46..7da1c5c 100644
--- a/cache.h
+++ b/cache.h
@@ -249,6 +249,7 @@ #define DEFAULT_ABBREV 7
 
 extern int get_sha1(const char *str, unsigned char *sha1);
 extern int get_sha1_hex(const char *hex, unsigned char *sha1);
+extern char *get_path_out_of_the_weird_syntax(const char *str);
 extern char *sha1_to_hex(const unsigned char *sha1);	/* static buffer result! */
 extern int read_ref(const char *filename, unsigned char *sha1);
 extern const char *resolve_ref(const char *path, unsigned char *sha1, int);
diff --git a/revision.c b/revision.c
index 874e349..fbd1458 100644
--- a/revision.c
+++ b/revision.c
@@ -908,6 +908,8 @@ int setup_revisions(int argc, const char
 		if (!seen_dashdash)
 			verify_non_filename(revs->prefix, arg);
 		object = get_reference(revs, arg, sha1, flags ^ local_flags);
+		if (object->type == OBJ_BLOB || object->type == OBJ_TREE)
+			arg = get_path_out_of_the_weird_syntax(arg);
 		add_pending_object(revs, object, arg);
 	}
 	if (show_merge)
diff --git a/sha1_name.c b/sha1_name.c
index 5fe8e5d..718ce1b 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -543,3 +543,15 @@ int get_sha1(const char *name, unsigned 
 	}
 	return ret;
 }
+
+/* get the path from sha1:path, :path and :[0-3]:path */
+char *get_path_out_of_the_weird_syntax(const char *str)
+{
+	if (strlen(str) >= 3 &&
+		str[0] == ':' && str[2] == ':' &&
+		str[1] >= '0' && str[2] <= '3')
+		return((char*)(str+3));
+	if (index(str, ':'))
+		return index(str, ':')+1;
+	return str;
+}
-- 
1.4.1.ga3e6

[PATCH] array index mixup

[Matthias Lederhofer]

---
Jakub Narebski <jnareb@gmail.com> wrote:
> git diff output for files specified by revision is somewhat unexpected. 
> 
>   $ git diff <revision_1>:<file> <revision_2>:<file>
> 
> outputs the following diff metainfo
> 
>   diff --git a/<revision_2>:<file> b/<revision_2>:<file>
>   index 5eabe06..2e87de4 100644
>   --- a/<revision_2>:<file>
>   +++ b/<revision_2>:<file>
> 
> Is it intended, or is it a bug? Looks like a bug to me...
I dunno if this is really an index mixup or was intended.  If this is
intended please add a comment what it's for.  (Without this you get
rename information, perhaps this is the reason.)
---
 builtin-diff.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-diff.c b/builtin-diff.c
index cb38f44..4d43a5c 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -136,7 +136,7 @@ static int builtin_diff_blobs(struct rev
 	stuff_change(&revs->diffopt,
 		     mode, mode,
 		     blob[1].sha1, blob[0].sha1,
-		     blob[0].name, blob[0].name);
+		     blob[1].name, blob[0].name);
 	diffcore_std(&revs->diffopt);
 	diff_flush(&revs->diffopt);
 	return 0;
-- 
1.4.1.ga3e6

Re: [PATCH] array index mixup

[Junio C Hamano]

Matthias Lederhofer <matled@gmx.net> writes:

> I dunno if this is really an index mixup or was intended.  If this is
> intended please add a comment what it's for.  (Without this you get
> rename information, perhaps this is the reason.)

That is exactly the reason -- it was a temporary workaround
which nobody noticed so far.

The right fix would involve updating diff_resolve_rename_copy so
that it does not rely on the comparison of path names (that
means DIFF_PAIR_RENAME() macro needs to change), and instead
mark the pairs synthesized in diffcore-rename as such, and use
that to tell if a pair is a result of rename/copy [*1*].

Your other patch (not the one to change the index of the array
used for labels, but the one that extracts the pathname out of
the syntax to name a blob by path in an arbitrary tree object)
could be safely applied when that happens.


[Footnote]

*1* If somebody wants to do this, one thing to watch out for is
matching up of broken pairs.  If a pair originally broken by
diffcore-break (because they were dissimilar enough according to
the option given to -B flag) are merged into one by
diffcore-rename (because they were similar enough according to
the option given to -M flag), we should _not_ say the resulting
pair is renamed.  In general, the threashold for breaking should
be lower than diffcore-rename to merge them for a sane use, so
this might be a non-issue in practice, though.