threads / rfc / 3704

[RFC] Silent File Mods Being Committed

Subject: [RFC] Silent File Mods Being Committed

## tl;dr

6 messages between Mar 23, 2006 and Mar 24, 2006.

replies: 5people: 3as markdown or json

Jon Loeliger· Mar 23, 2006, 04:04 UTC · lore
Folks,

I sort of got blindsided by committing (accidental) mode changes on a file that was also textually changed. Because it was textually changed, I happily expected 'git status' to show it as modified and subsequently 'git commit'-ed it.

Only after I 'git diff'-ed it later did I see that there was also a mode change on the file! Argh!

Secondarily, I think that the invisible file changes like this caused an eariler (yesterday-ish) confusion where an index looked clean, but was dirty due to stat issues and poor users (me) couldn't quite see why.

Originally, I was going to propose that git-ls-files be modified such that the mode and stat changes that ce_modified() correctly identifies as changing are somehow translated into output that "git status" shows the user per-file _in_addition_ to the normal "is modified" header. So I did this mod:

diff --git a/ls-files.c b/ls-files.c
index df25c8c..3e7e55d 100644
--- a/ls-files.c
+++ b/ls-files.c
@@ -450,7 +450,7 @@ static void show_killed_files(void)
 	}
 }
 
-static void show_ce_entry(const char *tag, struct cache_entry *ce)
+static void show_ce_entry(const char *tag, struct cache_entry *ce, int changed)
 {
 	int len = prefix_len;
 	int offset = prefix_offset;
@@ -482,6 +482,14 @@ static void show_ce_entry(const char *ta
 		fputs(tag, stdout);
 		write_name_quoted("", 0, ce->name + offset,
 				  line_terminator, stdout);
+		if (changed & (MTIME_CHANGED | CTIME_CHANGED)) {
+		    putchar(' ');
+		    putchar('T');
+		}
+		if (changed & MODE_CHANGED) {
+		    putchar(' ');
+		    putchar('M');
+		}
 		putchar(line_terminator);
 	}
 	else {
@@ -541,7 +549,7 @@ static void show_files(void)
 				continue;
 			if (show_unmerged && !ce_stage(ce))
 				continue;
-			show_ce_entry(ce_stage(ce) ? tag_unmerged : tag_cached, ce);
+			show_ce_entry(ce_stage(ce) ? tag_unmerged : tag_cached, ce, 0);
 		}
 	}
 	if (show_deleted | show_modified) {
@@ -553,9 +561,13 @@ static void show_files(void)
 				continue;
 			err = lstat(ce->name, &st);
 			if (show_deleted && err)
-				show_ce_entry(tag_removed, ce);
-			if (show_modified && ce_modified(ce, &st, 0))
-				show_ce_entry(tag_modified, ce);
+				show_ce_entry(tag_removed, ce, 0);
+			if (show_modified) {
+				int changed = ce_modified(ce, &st, 0);
+				if (changed)
+					show_ce_entry(tag_modified,
+						      ce, changed);
+			}
 		}
 	}
 }


And with that, "git ls-file -m" showed a trailing T or M to
indicate a "time" or a "mode" change happened on each file.

However, 'git status' didn't show that output....

And that is because it is driven by the diffcore instead!
So I _think_ diff_resolve_rename_copy() has to be consulted
to get that DIFF_STATUS_MODIFIED indicator.  Except that it is
shared with the SHA1 compare too:

                else if (memcmp(p->one->sha1, p->two->sha1, 20) ||
                         p->one->mode != p->two->mode)
                        p->status = DIFF_STATUS_MODIFIED;

But I haven't tracked it back to see how to propagate that
status back up to show_modified() in diff-files.c yet...

Maybe there is an easier way...?

jdl
Junio C Hamano· Mar 23, 2006, 05:13 UTC · re: Jon Loeliger · lore

Re: [RFC] Silent File Mods Being Committed

Jon Loeliger <jdl@jdl.com> writes:
Show 13 quoted lines
> However, 'git status' didn't show that output....
>
> And that is because it is driven by the diffcore instead!
> So I _think_ diff_resolve_rename_copy() has to be consulted
> to get that DIFF_STATUS_MODIFIED indicator.  Except that it is
> shared with the SHA1 compare too:
>
>                 else if (memcmp(p->one->sha1, p->two->sha1, 20) ||
>                          p->one->mode != p->two->mode)
>                         p->status = DIFF_STATUS_MODIFIED;
>
> But I haven't tracked it back to see how to propagate that
> status back up to show_modified() in diff-files.c yet...
I think the cleanest way would be either
 (1) define another output format to diffcore similar to
     --name-status, let that format perform most of what
     DIFF_FORMAT_NAME_STATUS does, and update
     diff.c:diff_flush_raw() to show the mode change as M+ (old
     was not executable but new is) or M- (the other way); or
 (2) audit all the scripts to make sure they do not get upset if
     we add trailing +/- to the status letter, and do that
     unconditionally, like the attached patch does.
If you go the latter route, you would get something like this:
        $ ./git-diff-files --abbrev
        :100644 100755 c0548ee... 0000000... M+	diff.c
	$ ./git-diff-files --name-status
        M+	diff.c
---
diff --git a/diff.c b/diff.c
index c0548ee..09b8f7e 100644
--- a/diff.c
+++ b/diff.c
@@ -1034,6 +1034,10 @@ static void diff_flush_raw(struct diff_f
 		status[0] = p->status;
 		status[1] = 0;
 	}
+
+	if (p->one->mode && p->two->mode && p->one->mode != p->two->mode)
+		strcat(status, (p->two->mode & 01) ? "+" : "-");
+
 	switch (p->status) {
 	case DIFF_STATUS_COPIED:
 	case DIFF_STATUS_RENAMED:
Petr Baudis· Mar 23, 2006, 21:47 UTC · re: Junio C Hamano · lore

Re: [RFC] Silent File Mods Being Committed

Dear diary, on Thu, Mar 23, 2006 at 06:13:21AM CET, I got a letter where Junio C Hamano <junkio@cox.net> said that...

>  (2) audit all the scripts to make sure they do not get upset if
>      we add trailing +/- to the status letter, and do that
>      unconditionally, like the attached patch does.

Cogito will get upset since we assume the mode field is one-char in our regexps, and when we don't, we compare the mode field with strings and that would obviously fail if you add random stuff to it.

Otherwise, I like this idea, though.
-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Right now I am having amnesia and deja-vu at the same time.  I think
I have forgotten this before.
Junio C Hamano· Mar 23, 2006, 23:32 UTC · re: Petr Baudis · lore

Re: [RFC] Silent File Mods Being Committed

Petr Baudis <pasky@suse.cz> writes:
Show 11 quoted lines
> Dear diary, on Thu, Mar 23, 2006 at 06:13:21AM CET, I got a letter
> where Junio C Hamano <junkio@cox.net> said that...
>>  (2) audit all the scripts to make sure they do not get upset if
>>      we add trailing +/- to the status letter, and do that
>>      unconditionally, like the attached patch does.
>
> Cogito will get upset since we assume the mode field is one-char in our
> regexps, and when we don't, we compare the mode field with strings and
> that would obviously fail if you add random stuff to it.
>
> Otherwise, I like this idea, though.

Likewise. If it was not obvious, I am not going to commit that myself. If jdl or somebody cares enough, he or she can prepare a patch to accomodate set of patches to git-core, Cogito and StGIT (at least these three should be covered) _and_ parrot my patch back at me. Hint, hint...

Junio C Hamano· Mar 23, 2006, 23:32 UTC · re: Petr Baudis · lore

Re: [RFC] Silent File Mods Being Committed

Petr Baudis <pasky@suse.cz> writes:
Show 11 quoted lines
> Dear diary, on Thu, Mar 23, 2006 at 06:13:21AM CET, I got a letter
> where Junio C Hamano <junkio@cox.net> said that...
>>  (2) audit all the scripts to make sure they do not get upset if
>>      we add trailing +/- to the status letter, and do that
>>      unconditionally, like the attached patch does.
>
> Cogito will get upset since we assume the mode field is one-char in our
> regexps, and when we don't, we compare the mode field with strings and
> that would obviously fail if you add random stuff to it.
>
> Otherwise, I like this idea, though.

Likewise. If it was not obvious, I am not going to commit that myself. If jdl or somebody cares enough, he or she can prepare a a set of patches to git-core, Cogito and StGIT (at least these three should be covered) to teach them the trailing +/- letter, _and_ parrot my patch back at me. Hint, hint...

Petr Baudis· Mar 24, 2006, 01:12 UTC · re: Junio C Hamano · lore

Re: [RFC] Silent File Mods Being Committed

Dear diary, on Fri, Mar 24, 2006 at 12:32:58AM CET, I got a letter where Junio C Hamano <junkio@cox.net> said that...

Show 19 quoted lines
> Petr Baudis <pasky@suse.cz> writes:
> 
> > Dear diary, on Thu, Mar 23, 2006 at 06:13:21AM CET, I got a letter
> > where Junio C Hamano <junkio@cox.net> said that...
> >>  (2) audit all the scripts to make sure they do not get upset if
> >>      we add trailing +/- to the status letter, and do that
> >>      unconditionally, like the attached patch does.
> >
> > Cogito will get upset since we assume the mode field is one-char in our
> > regexps, and when we don't, we compare the mode field with strings and
> > that would obviously fail if you add random stuff to it.
> >
> > Otherwise, I like this idea, though.
> 
> Likewise.  If it was not obvious, I am not going to commit that
> myself.  If jdl or somebody cares enough, he or she can prepare
> a a set of patches to git-core, Cogito and StGIT (at least these
> three should be covered) to teach them the trailing +/- letter,
> _and_ parrot my patch back at me.  Hint, hint...

Well, or it can just add the option to Core Git. Just unconditional implementation of (2) is no-go because it would break backwards compatibility, which is baaad.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Right now I am having amnesia and deja-vu at the same time.  I think
I have forgotten this before.

← back to recent threads