Re: git diff: add option for omitting the contents of deletes

Junio C Hamano [off-list ref] writes:

Michael J Gruber [off-list ref] writes:

quoted

Wasn't the pager invented for sifting through output which has to be
several pages, but not not for that which could be more concise? ;)

In fact, -D would be quite analogous to -M and -C in that respect.

There is a big difference: -M and -C lets your recipient reproduce the
state using the change you are trying to convey with the diff output in
either direction (iow, "apply -R" works), but your "-D" would not have
that property.

Having said that we have always valued "reversibility" and a casual -D is
not in line with that principle, I don't have a strong objection if the
new mode of operation is marked clearly as "nonusable if you are trying to
produce appliable diff (iow, don't send such a patch to mailing list--it
is for viewing purposes only)", treating it just like the --color-words
and the --stat options (there isn't even need to mark these as unusable
for that purpose, as people with common sense would be able to guess).

If we were to do this, it probably is a good idea to apply that for a
typechange patch (the one that is produced when a symlink turns into a
regular file and vice versa) as well.  It also might make sense to apply
the similar principle to shorten the output with -B when a rewrite patch
is expressed as a single hunk patch that removes everything old and then
adds everthing new.

Junio C Hamano venit, vidit, dixit 28.02.2011 00:07:

Junio C Hamano [off-list ref] writes:

quoted

Michael J Gruber [off-list ref] writes:

quoted

Wasn't the pager invented for sifting through output which has to be
several pages, but not not for that which could be more concise? ;)

In fact, -D would be quite analogous to -M and -C in that respect.

There is a big difference: -M and -C lets your recipient reproduce the
state using the change you are trying to convey with the diff output in
either direction (iow, "apply -R" works), but your "-D" would not have
that property.

Having said that we have always valued "reversibility" and a casual -D is

I didn't know, but I guess I haven't come across those issues yet.

not in line with that principle, I don't have a strong objection if the
new mode of operation is marked clearly as "nonusable if you are trying to
produce appliable diff (iow, don't send such a patch to mailing list--it
is for viewing purposes only)", treating it just like the --color-words
and the --stat options (there isn't even need to mark these as unusable
for that purpose, as people with common sense would be able to guess).

Yes, it is purely intended as "for viewing by humans". Even in the
forward direction saying "delete that path" is different from saying
"delete that content from that path", i.e. "diff -D" output may apply
without conflicts in cases where "diff" output does not.

That aspect is similar to -M and -C, though - unless we check the sha1
of the blobs before applying the patch (which would be possible for -D
also) - do we?

If we were to do this, it probably is a good idea to apply that for a
typechange patch (the one that is produced when a symlink turns into a
regular file and vice versa) as well.  It also might make sense to apply
the similar principle to shorten the output with -B when a rewrite patch
is expressed as a single hunk patch that removes everything old and then
adds everthing new.

Reminds me of my failed attempt to make the diff output for symlinks
more human-friendly. The latter can be solved with textconv, though.

Michael

Why not just make it reversible then?

$ git diff -M

diff --git a/foo b/bar
similarity index 100%
rename from foo
rename to bar

is nonreversible without git already (i.e. does not work with plain
patch AFAIK).

Adding

$ git diff -D

diff --git a/foo b/foo
deleted file mode 100644

delete foo

would be neither less nor more reversible -- it would also only work
with git apply
(assuming that apply is amended accordingly).

Cognitive burdens aside, the "delete foo" output is both
more explicit and shorter anyway. Occam's razor FTW :)!

Junio, I won't push this further, so it's a final call -- if you give -1,
then let -D fall to the colourless abyss of oblivion :), if +1 or +0,
I'll see if I can come up with a patch.

Best regards,
MS

On Mon, Feb 28, 2011 at 1:07 AM, Junio C Hamano [off-list ref] wrote:

Junio C Hamano [off-list ref] writes:

quoted

Michael J Gruber [off-list ref] writes:

quoted

Wasn't the pager invented for sifting through output which has to be
several pages, but not not for that which could be more concise? ;)

In fact, -D would be quite analogous to -M and -C in that respect.

There is a big difference: -M and -C lets your recipient reproduce the
state using the change you are trying to convey with the diff output in
either direction (iow, "apply -R" works), but your "-D" would not have
that property.

Having said that we have always valued "reversibility" and a casual -D is
not in line with that principle, I don't have a strong objection if the
new mode of operation is marked clearly as "nonusable if you are trying to
produce appliable diff (iow, don't send such a patch to mailing list--it
is for viewing purposes only)", treating it just like the --color-words
and the --stat options (there isn't even need to mark these as unusable
for that purpose, as people with common sense would be able to guess).

If we were to do this, it probably is a good idea to apply that for a
typechange patch (the one that is produced when a symlink turns into a
regular file and vice versa) as well.  It also might make sense to apply
the similar principle to shorten the output with -B when a rewrite patch
is expressed as a single hunk patch that removes everything old and then
adds everthing new.

On Mon, Feb 28, 2011 at 08:31:55AM +0100, Michael J Gruber wrote:

quoted

Junio C Hamano [off-list ref] writes:

quoted

There is a big difference: -M and -C lets your recipient reproduce the
state using the change you are trying to convey with the diff output in
either direction (iow, "apply -R" works), but your "-D" would not have
that property.

[...]

Yes, it is purely intended as "for viewing by humans". Even in the
forward direction saying "delete that path" is different from saying
"delete that content from that path", i.e. "diff -D" output may apply
without conflicts in cases where "diff" output does not.

That aspect is similar to -M and -C, though - unless we check the sha1
of the blobs before applying the patch (which would be possible for -D
also) - do we?

Yes, I think we do check the sha1s for a "-M" patch. And we should do so
for a "-D" patch, too. Which would make it just as likely to conflict as
a version with the actual patch content, _except_ that it relies on the
recipient having that sha1.

I don't see how "-D" is any less reversible than -M or -C, though. If I
get your -D patch, I use the index line to see that the blob went from
1234abcd to 0000000, check that we are at 1234abcd, and then delete the
file. To reverse it, I reinstate 1234abcd from nothing (and conflict if
the file exists). _Neither_ case works without the sha1.

So I think the problem is not about "this cannot be reversed" but about
"the recipient must have your sha1 to make sense of it, in either
direction". Which is the same case as with "-M" and "-C", and why we
have long cautioned about their use on mailing lists. But the right rule
is not "do not use on mailing lists" but rather "do not use on mailing
lists for projects where the recipients will not be using git to apply".

Reminds me of my failed attempt to make the diff output for symlinks
more human-friendly. The latter can be solved with textconv, though.

I am still carrying around my "symlinks as a special class of diff"
patches if you are interested:

  https://github.com/peff/git/tree/jk/userdiff-symlinks

-Peff

On Mon, Feb 28, 2011 at 07:17:26AM -0500, Jeff King wrote:

quoted

That aspect is similar to -M and -C, though - unless we check the sha1
of the blobs before applying the patch (which would be possible for -D
also) - do we?

Yes, I think we do check the sha1s for a "-M" patch. And we should do so
for a "-D" patch, too. Which would make it just as likely to conflict as
a version with the actual patch content, _except_ that it relies on the
recipient having that sha1.

I don't see how "-D" is any less reversible than -M or -C, though. If I
get your -D patch, I use the index line to see that the blob went from
1234abcd to 0000000, check that we are at 1234abcd, and then delete the
file. To reverse it, I reinstate 1234abcd from nothing (and conflict if
the file exists). _Neither_ case works without the sha1.

So I think the problem is not about "this cannot be reversed" but about
"the recipient must have your sha1 to make sense of it, in either
direction". Which is the same case as with "-M" and "-C", and why we
have long cautioned about their use on mailing lists. But the right rule
is not "do not use on mailing lists" but rather "do not use on mailing
lists for projects where the recipients will not be using git to apply".

Actually, thinking on this a bit more, I guess "-M" and "-C" are usable
without the sha1. In fact, we don't even provide it for a strict 100%
rename, and for a rename-with-patch, you can apply the patch, assuming
you have the original file in any form. So they are really about "is
your recipient using git", not "is your recipient using git _and_ will
he/she have the right sha1".

I do still think that in practice among git users that "-D" patches will
be able to be applied. You won't typically be sending a patch to delete
something that other people don't have. I would expect most uses of
"apply -R" to be reverting an existing delete, which means you will have
the sha1.

-Peff

Jeff King venit, vidit, dixit 28.02.2011 13:23:

On Mon, Feb 28, 2011 at 07:17:26AM -0500, Jeff King wrote:

quoted

quoted

That aspect is similar to -M and -C, though - unless we check the sha1
of the blobs before applying the patch (which would be possible for -D
also) - do we?

Yes, I think we do check the sha1s for a "-M" patch. And we should do so
for a "-D" patch, too. Which would make it just as likely to conflict as
a version with the actual patch content, _except_ that it relies on the
recipient having that sha1.

I don't see how "-D" is any less reversible than -M or -C, though. If I
get your -D patch, I use the index line to see that the blob went from
1234abcd to 0000000, check that we are at 1234abcd, and then delete the
file. To reverse it, I reinstate 1234abcd from nothing (and conflict if
the file exists). _Neither_ case works without the sha1.

So I think the problem is not about "this cannot be reversed" but about
"the recipient must have your sha1 to make sense of it, in either
direction". Which is the same case as with "-M" and "-C", and why we
have long cautioned about their use on mailing lists. But the right rule
is not "do not use on mailing lists" but rather "do not use on mailing
lists for projects where the recipients will not be using git to apply".

Actually, thinking on this a bit more, I guess "-M" and "-C" are usable
without the sha1. In fact, we don't even provide it for a strict 100%
rename, and for a rename-with-patch, you can apply the patch, assuming
you have the original file in any form. So they are really about "is
your recipient using git", not "is your recipient using git _and_ will
he/she have the right sha1".

$ git mv Makefile Dofile
$ git staged # yadayada

diff --git c/Makefile i/Dofile
similarity index 100%
rename from Makefile
rename to Dofile

Same with copy.

But that's not good, is it? I mean, Alice sends me her "copy patch" and
I send her my Makefile patch, both on top of the same base. We both
apply each other's patch cleanly. We end up with different "Dofile".
Checking the sha1 would prevent this. It's no surprise that patch
application is non-commutative, but shouldn't we catch this?

Michael

Jeff King venit, vidit, dixit 28.02.2011 13:17:

On Mon, Feb 28, 2011 at 08:31:55AM +0100, Michael J Gruber wrote:

[non symlink stuff snipped]

quoted

Reminds me of my failed attempt to make the diff output for symlinks
more human-friendly. The latter can be solved with textconv, though.

I am still carrying around my "symlinks as a special class of diff"
patches if you are interested:

  https://github.com/peff/git/tree/jk/userdiff-symlinks

This is marvelous (except for s/perl -pe/sed -e/, of course).

Is there anything left to do to get this in pu? (The reference to
"previous patch" may need to be more explicit.)

Michael

On Mon, Feb 28, 2011 at 01:32:35PM +0100, Michael J Gruber wrote:

Won't you either get a conflict or end up with the same Dofile? Clearly
you will have a Dofile with your Makefile changes, as you applied the
movement on top of your changes.

Alice will either:

  1. Apply not using rename detection (e.g., not using git, or using
     "git am" without "-3"). In this case, she gets a conflict because
     she no longer has Makefile.

  2. Apply using rename detection (e.g., via "git am -3"). In this case,
     we will notice the movement of Makefile to Dofile, and apply the
     patch to Dofile.

Still, I do wonder if we should be including an index line on a straight
rename patch. It lets the recipient check that what is being renamed is
what they have (IOW, it gives the same check that they would do if they
ahd the whole patch text). And then the recipient can decide how to
resolve the conflict.

-Peff

On Mon, Feb 28, 2011 at 01:42:06PM +0100, Michael J Gruber wrote:

quoted

I am still carrying around my "symlinks as a special class of diff"
patches if you are interested:

  https://github.com/peff/git/tree/jk/userdiff-symlinks

This is marvelous (except for s/perl -pe/sed -e/, of course).

I used perl because many older seds have trouble with files not ending
in newline.

Is there anything left to do to get this in pu? (The reference to
"previous patch" may need to be more explicit.)

See this subthread for discussion:

  http://article.gmane.org/gmane.comp.version-control.git/156760

On the one hand, there were doubts that anybody would actually want to
use it (though it looks like you may be the counterexample to that). On
the other hand, it may be that the solution does not go far enough, and
that .gitattributes should become aware of file types in its matching.
That would be more flexible, and it would also fix other broken areas
(like the fact that a symlink to foo.pdf might have its symlink text
merged as if it were a pdf).

-Peff

Jeff King venit, vidit, dixit 28.02.2011 13:59:

On Mon, Feb 28, 2011 at 01:32:35PM +0100, Michael J Gruber wrote:

quoted

quoted

Actually, thinking on this a bit more, I guess "-M" and "-C" are usable
without the sha1. In fact, we don't even provide it for a strict 100%
rename, and for a rename-with-patch, you can apply the patch, assuming
you have the original file in any form. So they are really about "is
your recipient using git", not "is your recipient using git _and_ will
he/she have the right sha1".

$ git mv Makefile Dofile
$ git staged # yadayada

diff --git c/Makefile i/Dofile
similarity index 100%
rename from Makefile
rename to Dofile

Same with copy.

But that's not good, is it? I mean, Alice sends me her "copy patch" and
I send her my Makefile patch, both on top of the same base. We both
apply each other's patch cleanly. We end up with different "Dofile".
Checking the sha1 would prevent this. It's no surprise that patch
application is non-commutative, but shouldn't we catch this?

Won't you either get a conflict or end up with the same Dofile? Clearly
you will have a Dofile with your Makefile changes, as you applied the
movement on top of your changes.

Alice will either:

  1. Apply not using rename detection (e.g., not using git, or using
     "git am" without "-3"). In this case, she gets a conflict because
     she no longer has Makefile.

When I said "copy patch" I actually meant a patch which records the copy
"Makefile -> Dofile". What is it today? Is it me? I know I wrote the
"mv" example first, but still :)

I mean, Alice:
cp Makefile Dofile
sends me a -C patch

I:
Break everything by hacking Makefile
send her a crappy patch

Both:
apply the received patch

Now I end up with a borked Makefile and a borked Dofile, but Alice still
has a good Dofile, and it's all my fault, so I don't deserve any better.
But still.

  2. Apply using rename detection (e.g., via "git am -3"). In this case,
     we will notice the movement of Makefile to Dofile, and apply the
     patch to Dofile.

Still, I do wonder if we should be including an index line on a straight
rename patch. It lets the recipient check that what is being renamed is
what they have (IOW, it gives the same check that they would do if they
ahd the whole patch text). And then the recipient can decide how to
resolve the conflict.

I think so. (Or make -f force it.)

This is orthogonal to the "-D" suggestion", but "-D" could write the
index line to start with.

Michael

This is a whether balloon patch (to check whether this balloons or not,
uhm). Hit it hard, not me ;)

It's part of the way to revive the old idea of making attributes depend
on file type. I am not following the old path

pattern is_symlink otherattr

because that would mean something which looks like an attribute
(is_symlink) but is not. Instead,

symlink:pattern attrs

is to specify attrs for pattern if it is a symlink. So, e.g.,

symlink:* diff=symlink

together with a nice diff.symlink.textconv displays symlinks nicely.
Most notable rough edges:

- git_checkattr() gets a new mode parameter. I've stuck it in where I
  could, 0 anywhere else. This is appropriate in many places (where we
know IS_REG a forteriori) but I'm not sure about all of them.

- We/someone could teach git_checkattr other prefixes.

- It is tested with one (1) symlink and works!

- I would bundle it with Jeff's doc and test ;)

- This is not a real commit message.

Signed-off-by: Michael J Gruber <redacted>
---
 archive.c              |    2 +-
 attr.c                 |    5 ++++-
 attr.h                 |    2 +-
 builtin/check-attr.c   |    7 ++++++-
 builtin/pack-objects.c |    2 +-
 convert.c              |    4 ++--
 diff.c                 |    5 ++---
 grep.c                 |    2 +-
 ll-merge.c             |    4 ++--
 userdiff.c             |    4 ++--
 userdiff.h             |    2 +-
 ws.c                   |    2 +-
 12 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/archive.c b/archive.c
index 1944ed4..2870f04 100644
--- a/archive.c
+++ b/archive.c

@@ -124,7 +124,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
 	path_without_prefix = path.buf + args->baselen;
 
 	setup_archive_check(check);
-	if (!git_checkattr(path_without_prefix, ARRAY_SIZE(check), check)) {
+	if (!git_checkattr(path_without_prefix, ARRAY_SIZE(check), check, mode)) {
 		if (ATTR_TRUE(check[0].value))
 			return 0;
 		convert = ATTR_TRUE(check[1].value);

diff --git a/attr.c b/attr.c
index 6aff695..c4481ac 100644
--- a/attr.c
+++ b/attr.c

@@ -708,7 +708,7 @@ static int macroexpand_one(int attr_nr, int rem)
 	return rem;
 }
 
-int git_checkattr(const char *path, int num, struct git_attr_check *check)
+int git_checkattr(const char *path, int num, struct git_attr_check *check, unsigned short mode)
 {
 	struct attr_stack *stk;
 	const char *cp;

@@ -718,6 +718,9 @@ int git_checkattr(const char *path, int num, struct git_attr_check *check)
 	for (i = 0; i < attr_nr; i++)
 		check_all_attr[i].value = ATTR__UNKNOWN;
 
+	if (S_ISLNK(mode))
+		path = prefix_filename("symlink:", strlen("symlink:"), path);
+	/* else if (IS_WHATEVER(mode)) do_what_you_feel_like */
 	pathlen = strlen(path);
 	cp = strrchr(path, '/');
 	if (!cp)

diff --git a/attr.h b/attr.h
index 8b3f19b..1af0bd4 100644
--- a/attr.h
+++ b/attr.h

@@ -29,7 +29,7 @@ struct git_attr_check {
 	const char *value;
 };
 
-int git_checkattr(const char *path, int, struct git_attr_check *);
+int git_checkattr(const char *path, int, struct git_attr_check *, unsigned short mode);
 
 enum git_attr_direction {
 	GIT_ATTR_CHECKIN,

diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 3016d29..30be2c2 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c

@@ -24,7 +24,12 @@ static void check_attr(int cnt, struct git_attr_check *check,
 	const char** name, const char *file)
 {
 	int j;
-	if (git_checkattr(file, cnt, check))
+	unsigned int mode = 0;
+	struct stat st;
+
+	if (!lstat(file, &st))
+		mode = st.st_mode;
+	if (git_checkattr(file, cnt, check, mode))
 		die("git_checkattr died");
 	for (j = 0; j < cnt; j++) {
 		const char *value = check[j].value;

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index b0503b2..4e9ff68 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c

@@ -633,7 +633,7 @@ static int no_try_delta(const char *path)
 	struct git_attr_check check[1];
 
 	setup_delta_attr_check(check);
-	if (git_checkattr(path, ARRAY_SIZE(check), check))
+	if (git_checkattr(path, ARRAY_SIZE(check), check, 0))
 		return 0;
 	if (ATTR_FALSE(check->value))
 		return 1;

diff --git a/convert.c b/convert.c
index d5aebed..d5c2b9c 100644
--- a/convert.c
+++ b/convert.c

@@ -736,7 +736,7 @@ int convert_to_git(const char *path, const char *src, size_t len,
 	const char *filter = NULL;
 
 	setup_convert_check(check);
-	if (!git_checkattr(path, ARRAY_SIZE(check), check)) {
+	if (!git_checkattr(path, ARRAY_SIZE(check), check, 0)) {
 		struct convert_driver *drv;
 		action = git_path_check_crlf(path, check + 4);
 		if (action == CRLF_GUESS)

@@ -773,7 +773,7 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
 	const char *filter = NULL;
 
 	setup_convert_check(check);
-	if (!git_checkattr(path, ARRAY_SIZE(check), check)) {
+	if (!git_checkattr(path, ARRAY_SIZE(check), check, 0)) {
 		struct convert_driver *drv;
 		action = git_path_check_crlf(path, check + 4);
 		if (action == CRLF_GUESS)

diff --git a/diff.c b/diff.c
index 6640857..22b5d94 100644
--- a/diff.c
+++ b/diff.c

@@ -1784,8 +1784,7 @@ static void diff_filespec_load_driver(struct diff_filespec *one)
 	if (one->driver)
 		return;
 
-	if (S_ISREG(one->mode))
-		one->driver = userdiff_find_by_path(one->path);
+	one->driver = userdiff_find_by_path(one->path, one->mode);
 
 	/* Fallback to default settings */
 	if (!one->driver)

@@ -2683,7 +2682,7 @@ static void run_diff_cmd(const char *pgm,
 	if (!DIFF_OPT_TST(o, ALLOW_EXTERNAL))
 		pgm = NULL;
 	else {
-		struct userdiff_driver *drv = userdiff_find_by_path(attr_path);
+		struct userdiff_driver *drv = userdiff_find_by_path(attr_path, two->mode);
 		if (drv && drv->external)
 			pgm = drv->external;
 	}

diff --git a/grep.c b/grep.c
index 63c4280..05a77fc 100644
--- a/grep.c
+++ b/grep.c

@@ -877,7 +877,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 	memset(&xecfg, 0, sizeof(xecfg));
 	if (opt->funcname && !opt->unmatch_name_only && !opt->status_only &&
 	    !opt->name_only && !binary_match_only && !collect_hits) {
-		struct userdiff_driver *drv = userdiff_find_by_path(name);
+		struct userdiff_driver *drv = userdiff_find_by_path(name, 0);
 		if (drv && drv->funcname.pattern) {
 			const struct userdiff_funcname *pe = &drv->funcname;
 			xdiff_set_find_func(&xecfg, pe->pattern, pe->cflags);

diff --git a/ll-merge.c b/ll-merge.c
index 6ce512e..3f351e3 100644
--- a/ll-merge.c
+++ b/ll-merge.c

@@ -330,7 +330,7 @@ static int git_path_check_merge(const char *path, struct git_attr_check check[2]
 		check[0].attr = git_attr("merge");
 		check[1].attr = git_attr("conflict-marker-size");
 	}
-	return git_checkattr(path, 2, check);
+	return git_checkattr(path, 2, check, 0);
 }
 
 static void normalize_file(mmfile_t *mm, const char *path)

@@ -387,7 +387,7 @@ int ll_merge_marker_size(const char *path)
 
 	if (!check.attr)
 		check.attr = git_attr("conflict-marker-size");
-	if (!git_checkattr(path, 1, &check) && check.value) {
+	if (!git_checkattr(path, 1, &check, 0) && check.value) {
 		marker_size = atoi(check.value);
 		if (marker_size <= 0)
 			marker_size = DEFAULT_CONFLICT_MARKER_SIZE;

diff --git a/userdiff.c b/userdiff.c
index 1ff4797..e5d0adf 100644
--- a/userdiff.c
+++ b/userdiff.c

@@ -245,7 +245,7 @@ struct userdiff_driver *userdiff_find_by_name(const char *name) {
 	return userdiff_find_by_namelen(name, len);
 }
 
-struct userdiff_driver *userdiff_find_by_path(const char *path)
+struct userdiff_driver *userdiff_find_by_path(const char *path, unsigned short mode)
 {
 	static struct git_attr *attr;
 	struct git_attr_check check;

@@ -256,7 +256,7 @@ struct userdiff_driver *userdiff_find_by_path(const char *path)
 
 	if (!path)
 		return NULL;
-	if (git_checkattr(path, 1, &check))
+	if (git_checkattr(path, 1, &check, mode))
 		return NULL;
 
 	if (ATTR_TRUE(check.value))

diff --git a/userdiff.h b/userdiff.h
index 942d594..e516f1d 100644
--- a/userdiff.h
+++ b/userdiff.h

@@ -21,6 +21,6 @@ struct userdiff_driver {
 
 int userdiff_config(const char *k, const char *v);
 struct userdiff_driver *userdiff_find_by_name(const char *name);
-struct userdiff_driver *userdiff_find_by_path(const char *path);
+struct userdiff_driver *userdiff_find_by_path(const char *path, unsigned short mode);
 
 #endif /* USERDIFF */

diff --git a/ws.c b/ws.c
index 9fb9b14..51929d3 100644
--- a/ws.c
+++ b/ws.c

@@ -88,7 +88,7 @@ unsigned whitespace_rule(const char *pathname)
 	struct git_attr_check attr_whitespace_rule;
 
 	setup_whitespace_attr_check(&attr_whitespace_rule);
-	if (!git_checkattr(pathname, 1, &attr_whitespace_rule)) {
+	if (!git_checkattr(pathname, 1, &attr_whitespace_rule, 0)) {
 		const char *value;
 
 		value = attr_whitespace_rule.value;

-- 
1.7.4.1.257.gb09fa

Mart Sõmermaa venit, vidit, dixit 28.02.2011 11:45:

That "i.e." is a misunderstanding:

I'm sure Junio meant that "git apply -R" should make sense on a path
like that.

You can easily undo (reverse) a rename without knowing the file
contents; undeleting a file is more difficult :)

To reverse it, we would need the sah1 lines, and the repo would need
that blob.

Cognitive burdens aside, the "delete foo" output is both
more explicit and shorter anyway. Occam's razor FTW :)!

Yes.

Junio, I won't push this further, so it's a final call -- if you give -1,
then let -D fall to the colourless abyss of oblivion :), if +1 or +0,
I'll see if I can come up with a patch.

You can always submit a patch, and you can never know what happens ;)

Best regards,
MS

Uhm, and please don't do that top post thing and the cc culling.
Readding J&J.

[quote snipped]

On Mon, Feb 28, 2011 at 04:26:18PM +0100, Michael J Gruber wrote:

This is a whether balloon patch (to check whether this balloons or not,
uhm). Hit it hard, not me ;)

Ouch.

It's part of the way to revive the old idea of making attributes depend
on file type. I am not following the old path

pattern is_symlink otherattr

because that would mean something which looks like an attribute
(is_symlink) but is not. Instead,

symlink:pattern attrs

is to specify attrs for pattern if it is a symlink. So, e.g.,

This is way better than what I proposed. From the user's perspective, it
is visually clearer that the symlink bit is part of the selector, and
not an attribute. And it syntactically disallows nonsense like:

  pattern is_symlink is_gitlink otherattr

The only downside is that it is technically a regression if somebody was
using gitattributes for the bizarrely named file "symlink:". It seems
pretty unlikely, but possibly we should be carving out a syntactic
namespace like:

  ^[a-z]+:

or even:

  ^[a-z]+(=[^:]*)?:

And that would later allow stuff like "submodule:" if people wanted to
attach specific bits to submodules (though perhaps it is not necessary,
because we have the entire separate .gitmodules file). I don't know if
there are other non-name elements people would be interested in
selecting on. So maybe that is over-engineering.

We also need some way of quoting. Duy has a 1.8.0 proposal to handle
this. If we're going to make a syntactic change to gitattributes for
this, it should probably be related (since the quoting mechanism is the
way you would fix it if you _are_ affected), and should probably follow
the same migration mechanism (it looks like there is talk of a "#
feature: foo bar" line).

See:

  http://article.gmane.org/gmane.comp.version-control.git/165970

I didn't test, but do we now assume that a pattern like "foo.*
diff=bar" will only match when foo is a regular file? I think that would
fix stuff like "*.pdf merge=pdf" when there is a symlink named
"file.pdf", which I'm pretty sure is currently broken (but did not
test).

OTOH, that is yet another behavior change if somebody had something like
"foo diff=symlink", which we would be breaking. So perhaps we will need
many "# feature" markers to make this right. :)

-Peff

On Mon, Feb 28, 2011 at 02:05:37PM +0100, Michael J Gruber wrote:

When I said "copy patch" I actually meant a patch which records the copy
"Makefile -> Dofile". What is it today? Is it me? I know I wrote the
"mv" example first, but still :)

Yeah, I looked at the "mv" example and totally glossed over the word
"copy" in your text.

I mean, Alice:
cp Makefile Dofile
sends me a -C patch

I:
Break everything by hacking Makefile
send her a crappy patch

Both:
apply the received patch

Now I end up with a borked Makefile and a borked Dofile, but Alice still
has a good Dofile, and it's all my fault, so I don't deserve any better.
But still.

OK, this is a much better example. Yes, you have different state at the
end, and there were no conflicts. I don't think we can resolve the
situation automagically, but I think it would at least be nice to
mention the conflict.

I guess the big question is how to mention it. Should it cause the patch
application to fail? I'm worried about that creating unnecessary false
positives for cases that are really quite harmless. Should patch
application just give a warning unless --strict-renames or something is
used? I dunno. This is one of those corner cases where we can see that
there is a potential problem, but it hasn't actually come up in
practice, so it's difficult to see what would be most useful in the real
world. Maybe that means we are wasting our time thinking about it. :)

This is orthogonal to the "-D" suggestion", but "-D" could write the
index line to start with.

Yeah. I had just assumed that "-D" would be the same as the current
text, minus the actual patch lines. IOW:

  diff --git a/Makefile b/Makefile
  deleted file mode 100644
  index c9ff69c..0000000

-Peff