parallel make problem with git

parallel make problem with git

[Michael S. Tsirkin]

Try this
$ git describe
v1.5.3-rc7
$make clean
$make -j 4
#... builds ok
$touch revision.c
$make -j 4
    CC revision.o
    AR libgit.a
    LINK git-fetch-pack
    LINK git-convert-objects
    LINK git-hash-object
    LINK git-index-pack
    LINK git-local-fetch
    LINK git-fast-import
    LINK git-daemon
    LINK git-mktag
    LINK git-merge-index
    LINK git-mktree
    LINK git-patch-id
    LINK git-peek-remote
    LINK git-receive-pack
    LINK git-send-pack
    LINK git-shell
    LINK git-show-index
    LINK git-ssh-fetch
    LINK git-ssh-upload
    LINK git-unpack-file
    LINK git-update-server-info
    LINK git-upload-pack
    LINK git-pack-redundant
    LINK git-var
    LINK git-merge-tree
    LINK git-imap-send
    LINK git-merge-recursive
    LINK git-ssh-pull
    LINK git-ssh-push
    LINK git-http-fetch
    LINK git-http-push
    LINK git
    LINK test-chmtime
gcc: test-chmtime.o: No such file or directory
make: *** [test-chmtime] Error 1
make: *** Waiting for unfinished jobs....

Any ideas?

-- 
MST

Re: parallel make problem with git

[Junio C Hamano]

"Michael S. Tsirkin" <mst@dev.mellanox.co.il> writes:

>     LINK test-chmtime
> gcc: test-chmtime.o: No such file or directory
> make: *** [test-chmtime] Error 1
> make: *** Waiting for unfinished jobs....
>
> Any ideas?

Some missing dependencies, apparently.

Re: parallel make problem with git

[Michael S. Tsirkin]

> Quoting Junio C Hamano <gitster@pobox.com>:
> Subject: Re: parallel make problem with git
> 
> "Michael S. Tsirkin" <mst@dev.mellanox.co.il> writes:
> 
> >     LINK test-chmtime
> > gcc: test-chmtime.o: No such file or directory
> > make: *** [test-chmtime] Error 1
> > make: *** Waiting for unfinished jobs....
> >
> > Any ideas?
> 
> Some missing dependencies, apparently.

Yes, I guessed as much :)

-- 
MST

[PATCH] fix parallel make problem

[Michael S. Tsirkin]

There seems to be a bug in parallel build with GNU Make 3.81beta4
which ships with Ubuntu Dapper:
$touch revision.c
$make -j 4
fails with
	LINK test-chmtime
	gcc: test-chmtime.o: No such file or directory
	make: *** [test-chmtime] Error 1

Even though test-chmtime depends on test-chmtime.o
Work around this by building test-* executables directly
from the appropriate .c file.

Signed-off-by: Michael S. Tsirkin <mst@dev.mellanox.co.il>

---
> Quoting Junio C Hamano <gitster@pobox.com>:
> Subject: Re: parallel make problem with git
> 
> "Michael S. Tsirkin" <mst@dev.mellanox.co.il> writes:
> 
> >     LINK test-chmtime
> > gcc: test-chmtime.o: No such file or directory
> > make: *** [test-chmtime] Error 1
> > make: *** Waiting for unfinished jobs....
> >
> > Any ideas?
> 
> Some missing dependencies, apparently.

Looks more like bug in make, to me: test-chmtime.o should have been
built by implicit rule, but isn't.

> make --version
GNU Make 3.81beta4

The following patch helps by building the test directly from .c file:

diff --git a/Makefile b/Makefile
index 4eb4637..d6b38b5 100644
--- a/Makefile
+++ b/Makefile
@@ -969,8 +969,8 @@ test-date$X: date.o ctype.o
 
 test-delta$X: diff-delta.o patch-delta.o
 
-test-%$X: test-%.o $(GITLIBS)
-	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
+test-%$X: test-%.c $(GITLIBS) GIT-CFLAGS
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.c,$^) $(LIBS)
 
 check-sha1:: test-sha1$X
 	./test-sha1.sh

-- 
MST

Re: [PATCH] fix parallel make problem

[Junio C Hamano]

How about this as a replacement?  I notice that after a
successful build all the test-*.o files are removed by make, and
somehow make seems to believe it is Ok not to recreate them.

---
 Makefile |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index 4eb4637..51af531 100644
--- a/Makefile
+++ b/Makefile
@@ -969,6 +969,8 @@ test-date$X: date.o ctype.o
 
 test-delta$X: diff-delta.o patch-delta.o
 
+.PRECIOUS: $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
+
 test-%$X: test-%.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
 
Warning: 1 path touched but unmodified. Consider running git-status.

Re: [PATCH] fix parallel make problem

[Michael S. Tsirkin]

> Quoting Junio C Hamano <gitster@pobox.com>:
> Subject: Re: [PATCH] fix parallel make problem
> 
> How about this as a replacement?  I notice that after a
> successful build all the test-*.o files are removed by make, and
> somehow make seems to believe it is Ok not to recreate them.

Yea. this works for me.

> ---
>  Makefile |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 4eb4637..51af531 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -969,6 +969,8 @@ test-date$X: date.o ctype.o
>  
>  test-delta$X: diff-delta.o patch-delta.o
>  
> +.PRECIOUS: $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
> +
>  test-%$X: test-%.o $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)

Add a comment here?
	
> Warning: 1 path touched but unmodified. Consider running git-status.

BTW, shouldn't the warning go to standard error?

-- 
MST

Re: [PATCH] fix parallel make problem

[Junio C Hamano]

"Michael S. Tsirkin" <mst@dev.mellanox.co.il> writes:

>> +.PRECIOUS: $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
>> +
>>  test-%$X: test-%.o $(GITLIBS)
>>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
>
> Add a comment here?

I did not see a particular need for that.  What would you say
there?

>> Warning: 1 path touched but unmodified. Consider running git-status.
>
> BTW, shouldn't the warning go to standard error?

No, usually you are under PAGER, so we need to send this to
stdout.  We do this only when we are generating textual diff
which will be consumed by patch or git-apply.  They both know
how to ignore such a non patch material.

Re: [PATCH] fix parallel make problem

[Michael S. Tsirkin]

> Quoting Junio C Hamano <gitster@pobox.com>:
> Subject: Re: [PATCH] fix parallel make problem
> 
> "Michael S. Tsirkin" <mst@dev.mellanox.co.il> writes:
> 
> >> +.PRECIOUS: $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
> >> +
> >>  test-%$X: test-%.o $(GITLIBS)
> >>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
> >
> > Add a comment here?
> 
> I did not see a particular need for that.  What would you say
> there?

That it's a work-around for make bug.

> >> Warning: 1 path touched but unmodified. Consider running git-status.
> >
> > BTW, shouldn't the warning go to standard error?
> 
> No, usually you are under PAGER, so we need to send this to
> stdout.  We do this only when we are generating textual diff
> which will be consumed by patch or git-apply.  They both know
> how to ignore such a non patch material.

So how did this end up in your mail?

-- 
MST

Re: [PATCH] fix parallel make problem

[Junio C Hamano]

"Michael S. Tsirkin" <mst@dev.mellanox.co.il> writes:

>> Quoting Junio C Hamano <gitster@pobox.com>:
>> Subject: Re: [PATCH] fix parallel make problem
>> 
>> "Michael S. Tsirkin" <mst@dev.mellanox.co.il> writes:
>> 
>> >> +.PRECIOUS: $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
>> >> +
>> >>  test-%$X: test-%.o $(GITLIBS)
>> >>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
>> >
>> > Add a comment here?
>> 
>> I did not see a particular need for that.  What would you say
>> there?
>
> That it's a work-around for make bug.

I would agree it is a make bug to barf like what we saw.  Even
though we allowed it to treat test-%.o files as intermediate
products and allowed them to be removed, it is not a good excuse
for make to forget rebuilding them.

But I also happen to think not marking test-%.o as precious was
a bug on our side.  We would want to keep the build by-product
to avoid recompilation, don't we?  And this additional line is
primarily about fixing that bug, which works the bug around as a
side effect.

> So how did this end up in your mail?

Because it is not a format-patch output.

I often run "git diff --stat -p HEAD" from inside MUA in order
to get the patch from my work tree, write a proposed commit
message, and then reset the change away without committing after
sending that message (yes I do not need "git stash" --- gmane
and vger are my stashes, Mwhhhaaaa).

Re: [PATCH] fix parallel make problem

[Michael S. Tsirkin]

> > So how did this end up in your mail?
> 
> Because it is not a format-patch output.
> 
> I often run "git diff --stat -p HEAD" from inside MUA in order
> to get the patch from my work tree, write a proposed commit
> message, and then reset the change away without committing after
> sending that message (yes I do not need "git stash" --- gmane
> and vger are my stashes, Mwhhhaaaa).

So maybe we can suppress the warning when the output is not a tty?


-- 
MST

Re: [PATCH] fix parallel make problem

[Junio C Hamano]

"Michael S. Tsirkin" <mst@dev.mellanox.co.il> writes:

>> > So how did this end up in your mail?
>> 
>> Because it is not a format-patch output.
>> 
>> I often run "git diff --stat -p HEAD" from inside MUA in order
>> to get the patch from my work tree, write a proposed commit
>> message, and then reset the change away without committing after
>> sending that message (yes I do not need "git stash" --- gmane
>> and vger are my stashes, Mwhhhaaaa).
>
> So maybe we can suppress the warning when the output is not a tty?

What's your point?

I did not even want to apply that "empty diff --git removal"
patch.  I certainly do _NOT_ want to suppress that replacement
warning anywhere.

You are seriously tempting me to revert the commit
fb13227e089f22dc31a3b1624559153821056848 (git-diff: squelch
"empty" diffs)...

Re: [PATCH] fix parallel make problem

[Michael S. Tsirkin]

> Quoting Junio C Hamano <gitster@pobox.com>:
> Subject: Re: [PATCH] fix parallel make problem
> 
> "Michael S. Tsirkin" <mst@dev.mellanox.co.il> writes:
> 
> >> > So how did this end up in your mail?
> >> 
> >> Because it is not a format-patch output.
> >> 
> >> I often run "git diff --stat -p HEAD" from inside MUA in order
> >> to get the patch from my work tree, write a proposed commit
> >> message, and then reset the change away without committing after
> >> sending that message (yes I do not need "git stash" --- gmane
> >> and vger are my stashes, Mwhhhaaaa).
> >
> > So maybe we can suppress the warning when the output is not a tty?
> 
> What's your point?

Well, git diff currently says "consider running git-status", and one
wanders why doesn't it just go ahead and run git status instead
of asking the user to do it.



-- 
MST

Re: [PATCH] fix parallel make problem

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Michael S. Tsirkin" <mst@dev.mellanox.co.il> writes:
> ...
>> So maybe we can suppress the warning when the output is not a tty?
>
> What's your point?
>
> I did not even want to apply that "empty diff --git removal"
> patch.  I certainly do _NOT_ want to suppress that replacement
> warning anywhere.
>
> You are seriously tempting me to revert the commit
> fb13227e089f22dc31a3b1624559153821056848 (git-diff: squelch
> "empty" diffs)...

This probably needs clarification.

That warning came because I was experimenting your reproduction
recipe of touching revision.c before building.  In earlier git,
it would have shown "diff --git a/revision.c b/revision.c"
without any actual diff text to remind me of that fact, and I
would have liked it better than the current "warning at the end"
behaviour for this particular case.  It was a reminder that my
recollection of what I did (and what my understanding of what
the state of my work tree is) matches the reality, but the new
"squelched" behaviour lost that value.

With the older git, whenever I sent out such a "this is how you
would do it" patch without committing [*1*], I removed those
"expected to be empty" diffs by hand after making a mental note
that these were to be expected and my understanding of where I
am matches reality.  It was a good thing.

The new "single warning at the end" was deliberately done to
reduce the "clutter" (which, this exchange is reminding me was
not clutter at all but valuable information) as a response to
requests from some people on the list, but because it was made
much less visible, it made me to miss it.

Having said that, I am _not_ going to revert that change at this
late in the game, but I am really tempted to add an option and a
configuration variable to revive the original behaviour (and of
course I'd set that configuration variable in _my_ repository).


[Footnote]

*1* Most of the patches I send out to the list are done that
way; I do "commit then send patch" much less often than "edit,
test, send 'diff HEAD' to the list and reset, intending to later
"am" it from the list if it is accepted favourably on the list".

Re: [PATCH] fix parallel make problem

[Jeff King]

On Fri, Aug 31, 2007 at 08:44:12AM -0700, Junio C Hamano wrote:

> I did not even want to apply that "empty diff --git removal"
> patch.  I certainly do _NOT_ want to suppress that replacement
> warning anywhere.
> 
> You are seriously tempting me to revert the commit
> fb13227e089f22dc31a3b1624559153821056848 (git-diff: squelch
> "empty" diffs)...

FWIW, I find the new message terribly ugly compared to the old behavior.
There have been many output changes that I didn't like at first, but for
which I held my tongue and eventually grew to like when they became more
familiar (e.g., the 'subject' line after git-commit).

But I just can't seem to find this one anything but ugly; everytime I
see it, I involuntarily cringe. Perhaps because it really looks like an
error message that accidentally got stuck in the diff output through
incompetent redirection of stdout/stderr.

I say this not to start a flame war (which is perhaps inevitable), but I
just wonder if others feel the same, now that they have had a chance to
get used to it.

-Peff

Re: [PATCH] fix parallel make problem

[Johannes Schindelin]

Hi,

On Fri, 31 Aug 2007, Michael S. Tsirkin wrote:

> > Quoting Junio C Hamano <gitster@pobox.com>:
> > Subject: Re: [PATCH] fix parallel make problem
> > 
> > "Michael S. Tsirkin" <mst@dev.mellanox.co.il> writes:
> > 
> > >> > So how did this end up in your mail?
> > >> 
> > >> Because it is not a format-patch output.
> > >> 
> > >> I often run "git diff --stat -p HEAD" from inside MUA in order
> > >> to get the patch from my work tree, write a proposed commit
> > >> message, and then reset the change away without committing after
> > >> sending that message (yes I do not need "git stash" --- gmane
> > >> and vger are my stashes, Mwhhhaaaa).
> > >
> > > So maybe we can suppress the warning when the output is not a tty?
> > 
> > What's your point?
> 
> Well, git diff currently says "consider running git-status", and one
> wanders why doesn't it just go ahead and run git status instead
> of asking the user to do it.

I knew why I was opposed to that change.  But others shouted louder, I 
guess.

Ciao,
Dscho

[PATCH] diff: resurrect the traditional empty "diff --git" behaviour

[Junio C Hamano]

The "Consier running git-status" warning message we experimented
during the 1.5.3 cycle turns out to be a bad idea.  It robbed
cache-dirty information from people who valued it, while still
asking users to run "update-index --refresh".  It was hoped that
the new behaviour would at least have some educational value,
but not showing the cache-dirty paths like before means the user
would not even know easily which paths are cache-dirty.

This commit reinstates the traditional behaviour as the default,
but with a twist.

If you set diff.autorefreshindex configuration variable, it
squelches the empty "diff --git" output, and at the end of the
command, it automatically runs "update-index --refresh" without
even bothering the user.  In other words, with the configuration
variable set, people who do not care about the cache-dirtyness
do not even have to see the warning.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 Jeff King <peff@peff.net> writes:

 > FWIW, I find the new message terribly ugly compared to the old behavior.
 > There have been many output changes that I didn't like at first, but for
 > which I held my tongue and eventually grew to like when they became more
 > familiar (e.g., the 'subject' line after git-commit).
 >
 > But I just can't seem to find this one anything but ugly; everytime I
 > see it, I involuntarily cringe. Perhaps because it really looks like an
 > error message that accidentally got stuck in the diff output through
 > incompetent redirection of stdout/stderr.
 >
 > I say this not to start a flame war (which is perhaps inevitable), but I
 > just wonder if others feel the same, now that they have had a chance to
 > get used to it.

 Same here.  This patch saw only very light testing, but I
 personally think is a sane thing to do before 1.5.3 final.

 builtin-diff.c |   31 ++++++++++++++++++++++++++-----
 cache.h        |    3 +++
 diff.c         |    5 +++++
 3 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/builtin-diff.c b/builtin-diff.c
index 6ed7b68..4ffbbad 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -188,6 +188,30 @@ void add_head(struct rev_info *revs)
 	add_pending_object(revs, obj, "HEAD");
 }
 
+static void refresh_index_quietly(void)
+{
+	struct lock_file *lock_file;
+	int fd;
+
+	lock_file = xcalloc(1, sizeof(struct lock_file));
+	fd = hold_locked_index(lock_file, 0);
+	if (fd < 0)
+		return;
+	discard_cache();
+	read_cache();
+	refresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);
+	if (active_cache_changed) {
+		if (write_cache(fd, active_cache, active_nr) ||
+		    close(fd) ||
+		    commit_locked_index(lock_file))
+			; /*
+			   * silently ignore it -- we haven't mucked
+			   * with the real index.
+			   */
+	}
+	rollback_lock_file(lock_file);
+}
+
 int cmd_diff(int argc, const char **argv, const char *prefix)
 {
 	int i;
@@ -222,7 +246,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	prefix = setup_git_directory_gently(&nongit);
 	git_config(git_diff_ui_config);
 	init_revisions(&rev, prefix);
-	rev.diffopt.skip_stat_unmatch = 1;
+	rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
 
 	if (!setup_diff_no_index(&rev, argc, argv, nongit, prefix))
 		argc = 0;
@@ -348,9 +372,6 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 
 	if ((rev.diffopt.output_format & DIFF_FORMAT_PATCH)
 	    && (1 < rev.diffopt.skip_stat_unmatch))
-		printf("Warning: %d path%s touched but unmodified. "
-		       "Consider running git-status.\n",
-		       rev.diffopt.skip_stat_unmatch - 1,
-		       rev.diffopt.skip_stat_unmatch == 2 ? "" : "s");
+		refresh_index_quietly();
 	return result;
 }
diff --git a/cache.h b/cache.h
index c7e00e7..70abbd5 100644
--- a/cache.h
+++ b/cache.h
@@ -594,6 +594,9 @@ extern char *convert_to_git(const char *path, const char *src, unsigned long *si
 extern char *convert_to_working_tree(const char *path, const char *src, unsigned long *sizep);
 extern void *convert_sha1_file(const char *path, const unsigned char *sha1, unsigned int mode, enum object_type *type, unsigned long *size);
 
+/* diff.c */
+extern int diff_auto_refresh_index;
+
 /* match-trees.c */
 void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, int);
 
diff --git a/diff.c b/diff.c
index a7e7671..75d95da 100644
--- a/diff.c
+++ b/diff.c
@@ -19,6 +19,7 @@
 static int diff_detect_rename_default;
 static int diff_rename_limit_default = -1;
 static int diff_use_color_default;
+int diff_auto_refresh_index;
 
 static char diff_colors[][COLOR_MAXLEN] = {
 	"\033[m",	/* reset */
@@ -166,6 +167,10 @@ int git_diff_ui_config(const char *var, const char *value)
 			diff_detect_rename_default = DIFF_DETECT_RENAME;
 		return 0;
 	}
+	if (!strcmp(var, "diff.autorefreshindex")) {
+		diff_auto_refresh_index = git_config_bool(var, value);
+		return 0;
+	}
 	if (!prefixcmp(var, "diff.")) {
 		const char *ep = strrchr(var, '.');
 

Re: [PATCH] diff: resurrect the traditional empty "diff --git" behaviour

[Jeff King]

On Fri, Aug 31, 2007 at 01:13:42PM -0700, Junio C Hamano wrote:

> If you set diff.autorefreshindex configuration variable, it
> squelches the empty "diff --git" output, and at the end of the
> command, it automatically runs "update-index --refresh" without
> even bothering the user.  In other words, with the configuration
> variable set, people who do not care about the cache-dirtyness
> do not even have to see the warning.

Nice. This is much more sane behavior, IMHO, and I think it should make
everyone happy.

>  Same here.  This patch saw only very light testing, but I
>  personally think is a sane thing to do before 1.5.3 final.

Passes my light testing as well, but I have a feeling we just tested the
same things...;)

One question on the implementation (and remember that I am somewhat
ignorant of the structure of this part of the code, so the answer may be
"it's too ugly"): is there a good reason to refresh _after_ the diff? It
seems like when we are looking through the working tree and index the
first time, we notice that the stat information doesn't match; why can't
we update it then? That would save an extra working tree traversal.

-Peff

Re: [PATCH] diff: resurrect the traditional empty "diff --git" behaviour

[Johannes Schindelin]

Hi,

On Fri, 31 Aug 2007, Jeff King wrote:

> On Fri, Aug 31, 2007 at 01:13:42PM -0700, Junio C Hamano wrote:
> 
> > If you set diff.autorefreshindex configuration variable, it
> > squelches the empty "diff --git" output, and at the end of the
> > command, it automatically runs "update-index --refresh" without
> > even bothering the user.  In other words, with the configuration
> > variable set, people who do not care about the cache-dirtyness
> > do not even have to see the warning.
> 
> Nice. This is much more sane behavior, IMHO, and I think it should make 
> everyone happy.

I could even imagine that this will eventually become the standard 
behaviour.

> >  Same here.  This patch saw only very light testing, but I
> >  personally think is a sane thing to do before 1.5.3 final.
> 
> Passes my light testing as well, but I have a feeling we just tested the
> same things...;)
> 
> One question on the implementation (and remember that I am somewhat
> ignorant of the structure of this part of the code, so the answer may be
> "it's too ugly"): is there a good reason to refresh _after_ the diff?

We do not need to do it always.  After the diff, we know if the index 
needs refreshing.  Before, we don't.

> It seems like when we are looking through the working tree and index the 
> first time, we notice that the stat information doesn't match; why can't 
> we update it then? That would save an extra working tree traversal.

But that would be intrusive in the diff machinery IMHO.  It should stay as 
read-only as possible.

BTW I was a little concerned that the locking would fail in a read-only 
setup, and that git would die(), but that has been taken care of, so I 
have no objections left.

Thanks, Junio.

Ciao,
Dscho

Re: [PATCH] diff: resurrect the traditional empty "diff --git" behaviour

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Fri, 31 Aug 2007, Jeff King wrote:
>
>> On Fri, Aug 31, 2007 at 01:13:42PM -0700, Junio C Hamano wrote:
>> 
>> > If you set diff.autorefreshindex configuration variable, it
>> > squelches the empty "diff --git" output, and at the end of the
>> > command, it automatically runs "update-index --refresh" without
>> > even bothering the user.  In other words, with the configuration
>> > variable set, people who do not care about the cache-dirtyness
>> > do not even have to see the warning.
>> 
>> Nice. This is much more sane behavior, IMHO, and I think it should make 
>> everyone happy.
>
> I could even imagine that this will eventually become the standard 
> behaviour.

You sound as if you _like_ that behaviour...

Re: [PATCH] diff: resurrect the traditional empty "diff --git" behaviour

[David Kastrup]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Fri, 31 Aug 2007, Jeff King wrote:
>
>> On Fri, Aug 31, 2007 at 01:13:42PM -0700, Junio C Hamano wrote:
>> 
>> > If you set diff.autorefreshindex configuration variable, it
>> > squelches the empty "diff --git" output, and at the end of the
>> > command, it automatically runs "update-index --refresh" without
>> > even bothering the user.  In other words, with the configuration
>> > variable set, people who do not care about the cache-dirtyness
>> > do not even have to see the warning.
>> 
>> Nice. This is much more sane behavior, IMHO, and I think it should make 
>> everyone happy.
>
> I could even imagine that this will eventually become the standard 
> behaviour.
>
>> >  Same here.  This patch saw only very light testing, but I
>> >  personally think is a sane thing to do before 1.5.3 final.
>> 
>> Passes my light testing as well, but I have a feeling we just tested the
>> same things...;)
>> 
>> One question on the implementation (and remember that I am somewhat
>> ignorant of the structure of this part of the code, so the answer may be
>> "it's too ugly"): is there a good reason to refresh _after_ the diff?
>
> We do not need to do it always.  After the diff, we know if the
> index needs refreshing.  Before, we don't.

Hm.  At the moment where it is first noticed, it should be still
possible to start a refresh.  Is there a particular gain to be
expected?  One thing I could think of is that when using a pager, the
diff might often die of SIGPIPE before being able to refresh.

>> It seems like when we are looking through the working tree and
>> index the first time, we notice that the stat information doesn't
>> match; why can't we update it then? That would save an extra
>> working tree traversal.
>
> But that would be intrusive in the diff machinery IMHO.  It should
> stay as read-only as possible.

Hm.  Not sure where the gain is in that, if a refresh is done, anyway.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] diff: resurrect the traditional empty "diff --git" behaviour

[Johannes Schindelin]

Hi,

On Fri, 31 Aug 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Fri, 31 Aug 2007, Jeff King wrote:
> >
> >> On Fri, Aug 31, 2007 at 01:13:42PM -0700, Junio C Hamano wrote:
> >> 
> >> > If you set diff.autorefreshindex configuration variable, it
> >> > squelches the empty "diff --git" output, and at the end of the
> >> > command, it automatically runs "update-index --refresh" without
> >> > even bothering the user.  In other words, with the configuration
> >> > variable set, people who do not care about the cache-dirtyness
> >> > do not even have to see the warning.
> >> 
> >> Nice. This is much more sane behavior, IMHO, and I think it should make 
> >> everyone happy.
> >
> > I could even imagine that this will eventually become the standard 
> > behaviour.
> 
> You sound as if you _like_ that behaviour...

Heh.  Almost.  I'd like to believe that these complaints would stop.

Ciao,
Dscho

Re: [PATCH] diff: resurrect the traditional empty "diff --git" behaviour

[Alex Riesen]

Junio C Hamano, Fri, Aug 31, 2007 22:13:42 +0200:
> The "Consier running git-status" warning message we experimented
> during the 1.5.3 cycle turns out to be a bad idea.  It robbed
> cache-dirty information from people who valued it, while still
> asking users to run "update-index --refresh".  It was hoped that
> the new behaviour would at least have some educational value,
> but not showing the cache-dirty paths like before means the user
> would not even know easily which paths are cache-dirty.
> 
> This commit reinstates the traditional behaviour as the default,
> but with a twist.
> 
> If you set diff.autorefreshindex configuration variable, it
> squelches the empty "diff --git" output, and at the end of the
> command, it automatically runs "update-index --refresh" without
> even bothering the user.  In other words, with the configuration
> variable set, people who do not care about the cache-dirtyness
> do not even have to see the warning.

I like this change.

So far my attempts to explain that warning to myself always left an
uneasy feeling of me having tricked myself into believing in its
usefullness.

Re: [PATCH] diff: resurrect the traditional empty "diff --git" behaviour

[Steven Grimm]

Junio C Hamano wrote:
> This commit reinstates the traditional behaviour as the default,
> but with a twist.
>
> If you set diff.autorefreshindex configuration variable, it
> squelches the empty "diff --git" output, and at the end of the
> command, it automatically runs "update-index --refresh" without
> even bothering the user.  In other words, with the configuration
> variable set, people who do not care about the cache-dirtyness
> do not even have to see the warning.
>   

As the person who submitted the patch you're reversing with this, I 
agree 100% this is the better approach. Having the system self-heal is 
far preferable to requiring user action.

I would vote for reversing the sense of that config variable, actually: 
my guess is that most users, especially new ones who won't necessarily 
know about the config variable, would rather have git just keep itself 
healthy without user intervention. Having to manually update the index 
is (by virtue of requiring you to run a command that you wouldn't 
otherwise need to run) a mode more suited to advanced / experienced 
users, who are more likely to already be comfortable with the idea of 
flipping config switches. Making the more novice-friendly mode require 
that you know how to set a particular configuration variable doesn't 
seem right to me.

-Steve

Re: [PATCH] diff: resurrect the traditional empty "diff --git" behaviour

[Junio C Hamano]

Steven Grimm <koreth@midwinter.com> writes:

> Junio C Hamano wrote:
>> This commit reinstates the traditional behaviour as the default,
>> but with a twist.
>>
>> If you set diff.autorefreshindex configuration variable, it
>> squelches the empty "diff --git" output, and at the end of the
>> command, it automatically runs "update-index --refresh" without
>> even bothering the user.  In other words, with the configuration
>> variable set, people who do not care about the cache-dirtyness
>> do not even have to see the warning.
>
> As the person who submitted the patch you're reversing with this, I
> agree 100% this is the better approach. Having the system self-heal is
> far preferable to requiring user action.
>
> I would vote for reversing the sense of that config variable,

(grudgingly...) Ok.

Old timers like myself have been warned on the list that
diff output will get quieter, so why not.

This is on top of the previous one in this thread.



diff --git a/Documentation/config.txt b/Documentation/config.txt
index 903610f..cf7617a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -396,6 +396,16 @@ color.status.<slot>::
 commit.template::
 	Specify a file to use as the template for new commit messages.
 
+diff.autorefreshindex::
+	When using `git diff` to compare with work tree
+	files, do not consider stat-only change as changed.
+	Instead, silently run `git update-index --refresh` to
+	update the cached stat information for paths whose
+	contents in the work tree match the contents in the
+	index.  This option defaults to true.  Note that this
+	affects only `git diff` Porcelain, and not lower level
+	`diff` commands, such as `git diff-files`.
+
 diff.renameLimit::
 	The number of files to consider when performing the copy/rename
 	detection; equivalent to the git diff option '-l'.
diff --git a/diff.c b/diff.c
index 75d95da..0d30d05 100644
--- a/diff.c
+++ b/diff.c
@@ -19,7 +19,7 @@
 static int diff_detect_rename_default;
 static int diff_rename_limit_default = -1;
 static int diff_use_color_default;
-int diff_auto_refresh_index;
+int diff_auto_refresh_index = 1;
 
 static char diff_colors[][COLOR_MAXLEN] = {
 	"\033[m",	/* reset */

Re: [PATCH] diff: resurrect the traditional empty "diff --git" behaviour

[Matthieu Moy]

I fully agree with the two patches (that's what I've been arguing for
hours last time we talked about it, so no big surprise ;-) ).

One documentation nitpick :

Junio C Hamano <gitster@pobox.com> writes:

> +diff.autorefreshindex::
> +	When using `git diff` to compare with work tree
> +	files, do not consider stat-only change as changed.
> +	Instead, silently run `git update-index --refresh`

I'd rather avoid talking about plumbing in the documentation of
porcelain, so I'd say "silently refreshes the index's stat
information".

But I'm arguably wrong on that point, I let you decide.

Thanks,

-- 
Matthieu

Re: [PATCH] diff: resurrect the traditional empty "diff --git" behaviour

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> I'd rather avoid talking about plumbing in the documentation of
> porcelain, so I'd say "silently refreshes the index's stat
> information".

Sounds like a much better wording.