Re: v1.5.4 plans

Jeff King [off-list ref] writes:

A few regressions that you did not mention, but I think should be
addressed before 1.5.4:

 - extra newline in builtin-commit output. You found a case that
   needs it, but fixing it is non-trivial, and I wanted to get your
   input before preparing a patch. See

     http://mid.gmane.org/20071203075357.GB3614@sigill.intra.peff.net

I am actually becoming somewhat fond of the newline that makes the end
of a session that led to a commit stand out ;-). IOW, I was wondering if
we can have another for a merge commit case.

But I suspect that it amounts to the change in the same area and of
similar complexity.

 - git-clean's handling of directory wildcards. I didn't get a response
   to

     http://mid.gmane.org/20071206043247.GC5499@coredump.intra.peff.net

   I suspect there are still some bugs lurking in there, but it's hard
   to say because I don't know what the behavior _should_ be (there are
   some test cases in that email).

The last time I looked at the "directory" side of builtin-clean.c, I had
to quickly reach for my barf bag.  I never use "git clean" without "-n"
and I never ever use "git clean" with "-d"; I do not have any idea what
behaviour when given "-d" would be useful.  AFAIU, the scripted version
did not have clear semantics either.

Another thing that irritates me is it talks about not removing a
directory when run "git clean -n" (without -d).  I did not ask it to
remove directories, so I did not expect it to talk about it not doing
what I did not ask it to.

And perhaps not a regression, but I think we should bring git-svn's
handling of color.* in line with the changes to the rest of the code
before 1.5.4. I posted a "last resort" patch, but I think with your
changes to "git config --colorbool" it might be possible to use that.
I'll try to work up a new patch.

Thanks for a reminder.  Anything else?

On Mon, Dec 10, 2007 at 05:27:17PM -0800, Junio C Hamano wrote:

quoted

And perhaps not a regression, but I think we should bring git-svn's
handling of color.* in line with the changes to the rest of the code
before 1.5.4. I posted a "last resort" patch, but I think with your
changes to "git config --colorbool" it might be possible to use that.
I'll try to work up a new patch.

Thanks for a reminder.  Anything else?

2-patch series will follow momentarily. 1/2 gives --get-colorbool the
necessary information for implementing color.pager, and 2/2 fixes
git-svn.

Very light testing by me, since I'm not actually a git-svn user, but it
does pass the test scripts. Acks from svn-using people would be nice.

-Peff

Subject: [PATCH 1/2] Support GIT_PAGER_IN_USE environment variable

When deciding whether or not to turn on automatic color
support, git_config_colorbool checks whether stdout is a
tty. However, because we run a pager, if stdout is not a
tty, we must check whether it is because we started the
pager. This used to be done by checking the pager_in_use
variable.

This variable was set only when the git program being run
started the pager; there was no way for an external program
running git indicate that it had already started a pager.
This patch allows a program to set GIT_PAGER_IN_USE to a
true value to indicate that even though stdout is not a tty,
it is because a pager is being used.

Signed-off-by: Jeff King <redacted>
---

A few notes:

We could also just put the color.pager logic in git-svn, or in Git.pm,
and have it impact the stdout_is_tty argument; but the whole point of
--get-colorbool is to consolidate that logic.

We convert pager_in_use to a function; we could also just set the
variable early on, but I think this lazy evaluation is more robust.

This might have uses besides --get-colorbool (e.g., wrapper scripts
which start their own pager can still have git sub-commands understand
whether to turn on color).

 cache.h       |    2 +-
 color.c       |    2 +-
 environment.c |    1 -
 pager.c       |   15 ++++++++++++++-
 4 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index 1bcb3df..27d90fe 100644
--- a/cache.h
+++ b/cache.h

@@ -608,7 +608,7 @@ extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char
 /* pager.c */
 extern void setup_pager(void);
 extern char *pager_program;
-extern int pager_in_use;
+extern int pager_in_use(void);
 extern int pager_use_color;
 
 extern char *editor_program;

diff --git a/color.c b/color.c
index 7bd424a..7f66c29 100644
--- a/color.c
+++ b/color.c

@@ -135,7 +135,7 @@ int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)
  auto_color:
 	if (stdout_is_tty < 0)
 		stdout_is_tty = isatty(1);
-	if (stdout_is_tty || (pager_in_use && pager_use_color)) {
+	if (stdout_is_tty || (pager_in_use() && pager_use_color)) {
 		char *term = getenv("TERM");
 		if (term && strcmp(term, "dumb"))
 			return 1;

diff --git a/environment.c b/environment.c
index f3e3d41..18a1c4e 100644
--- a/environment.c
+++ b/environment.c

@@ -31,7 +31,6 @@ size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;
 size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;
 size_t delta_base_cache_limit = 16 * 1024 * 1024;
 char *pager_program;
-int pager_in_use;
 int pager_use_color = 1;
 char *editor_program;
 char *excludes_file;

diff --git a/pager.c b/pager.c
index fb7a1a6..0376953 100644
--- a/pager.c
+++ b/pager.c

@@ -5,6 +5,8 @@
  * something different on Windows, for example.
  */
 
+static int spawned_pager;
+
 static void run_pager(const char *pager)
 {
 	/*

@@ -41,7 +43,7 @@ void setup_pager(void)
 	else if (!*pager || !strcmp(pager, "cat"))
 		return;
 
-	pager_in_use = 1; /* means we are emitting to terminal */
+	spawned_pager = 1; /* means we are emitting to terminal */
 
 	if (pipe(fd) < 0)
 		return;

@@ -70,3 +72,14 @@ void setup_pager(void)
 	die("unable to execute pager '%s'", pager);
 	exit(255);
 }
+
+int pager_in_use(void)
+{
+	const char *env;
+
+	if (spawned_pager)
+		return 1;
+
+	env = getenv("GIT_PAGER_IN_USE");
+	return env ? git_config_bool("GIT_PAGER_IN_USE", env) : 0;
+}

-- 
1.5.3.7.2230.g796d07-dirty

git-config recently learned a --get-colorbool option. By
using it, we will get the same color=auto behavior that
other git commands have.

Specifically, this fixes the case where "color.diff = true"
meant "always" in git-svn, but "auto" in other programs.

Signed-off-by: Jeff King <redacted>
---
 git-svn.perl |   35 ++---------------------------------
 1 files changed, 2 insertions(+), 33 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 9f884eb..1c42c55 100755
--- a/git-svn.perl
+++ b/git-svn.perl

@@ -3969,39 +3969,7 @@ sub cmt_showable {
 }
 
 sub log_use_color {
-	return 1 if $color;
-	my ($dc, $dcvar);
-	$dcvar = 'color.diff';
-	$dc = `git-config --get $dcvar`;
-	if ($dc eq '') {
-		# nothing at all; fallback to "diff.color"
-		$dcvar = 'diff.color';
-		$dc = `git-config --get $dcvar`;
-	}
-	chomp($dc);
-	if ($dc eq 'auto') {
-		my $pc;
-		$pc = `git-config --get color.pager`;
-		if ($pc eq '') {
-			# does not have it -- fallback to pager.color
-			$pc = `git-config --bool --get pager.color`;
-		}
-		else {
-			$pc = `git-config --bool --get color.pager`;
-			if ($?) {
-				$pc = 'false';
-			}
-		}
-		chomp($pc);
-		if (-t *STDOUT || (defined $pager && $pc eq 'true')) {
-			return ($ENV{TERM} && $ENV{TERM} ne 'dumb');
-		}
-		return 0;
-	}
-	return 0 if $dc eq 'never';
-	return 1 if $dc eq 'always';
-	chomp($dc = `git-config --bool --get $dcvar`);
-	return ($dc eq 'true');
+	return $color || Git->repository->get_colorbool('color.diff');
 }
 
 sub git_svn_log_cmd {

@@ -4060,6 +4028,7 @@ sub config_pager {
 	} elsif (length $pager == 0 || $pager eq 'cat') {
 		$pager = undef;
 	}
+	$ENV{GIT_PAGER_IN_USE} = defined($pager);
 }
 
 sub run_pager {

-- 
1.5.3.7.2230.g796d07-dirty

On Mon, Dec 10, 2007 at 05:27:17PM -0800, Junio C Hamano wrote:

quoted

   I suspect there are still some bugs lurking in there, but it's hard
   to say because I don't know what the behavior _should_ be (there are
   some test cases in that email).

The last time I looked at the "directory" side of builtin-clean.c, I had
to quickly reach for my barf bag.  I never use "git clean" without "-n"
and I never ever use "git clean" with "-d"; I do not have any idea what
behaviour when given "-d" would be useful.  AFAIU, the scripted version
did not have clear semantics either.

I had the same feeling. I am tempted to leave it, then. The
non-intuitive behavior I managed to trigger was:

  - _only_ when using git pathspec matching like "git clean -n '*.ext'"
  - confusing in a safe way (trying to remove 'dir.ext' with '*.ext'
    will accidentally not happen, rather than accidentally happening)

So unless somebody complains, it is probably not a big problem,
and I think fixing it will require mucking with pathspec and dir
matching internals, which would be nice not to do right before v1.5.4.

OTOH, leaving something that is broken and just hoping nobody will
complain feels kind of wrong. :)

-Peff

Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

 - git-clean's handling of directory wildcards. I didn't get a response
   to

     http://mid.gmane.org/20071206043247.GC5499@coredump.intra.peff.net

   I suspect there are still some bugs lurking in there, but it's hard
   to say because I don't know what the behavior _should_ be (there are
   some test cases in that email).

The last time I looked at the "directory" side of builtin-clean.c, I had
to quickly reach for my barf bag.  I never use "git clean" without "-n"
and I never ever use "git clean" with "-d"; I do not have any idea what
behaviour when given "-d" would be useful.

When you have a trash directory without any tracked files, clean will not
by default descend into that directory and thus won't remove neither files
nor directory. I frequently use one for automated testing, much like git's
trash repository, but the only time I do "git clean -d" is when building
things on a release-server with the repository checked out. It's faster
than "make distclean", and not all of our projects have a Makefile to begin
with. Tacking "git clean -d" at the end of test-scripts makes it simple to
remove all excess cruft in one go.

So in short, git clean -d can be useful. I have no idea when "git clean dir"
would be though.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

On Mon, Dec 10, 2007 at 05:27:17PM -0800, Junio C Hamano wrote:

Thanks for a reminder.  Anything else?

This bugfix has been sitting in my repo for a few weeks.  When it last
appeared, you asked if the other code paths needed a similar fix, and I
verified that they did not, so I think it is complete as-is.

-- >8 --
git-clone: print an error message when trying to clone empty repo

Previously, cloning an empty repository looked like this:

$ (mkdir parent && cd parent && git --bare init)
$ git-clone parent child
Initialized empty Git repository in /home/peff/clone/child/.git/
$ cd child
-bash: cd: child: No such file or directory
$ echo 'wtf?' | mail git@vger.kernel.org

Now we at least report that the clone was not successful.

Signed-off-by: Jeff King <redacted>
---
 git-clone.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/git-clone.sh b/git-clone.sh
index ecf9d89..96a356d 100755
--- a/git-clone.sh
+++ b/git-clone.sh

@@ -297,7 +297,8 @@ yes)
 				      find objects -type f -print | sed -e 1q)
 			# objects directory should not be empty because
 			# we are cloning!
-			test -f "$repo/$sample_file" || exit
+			test -f "$repo/$sample_file" ||
+				die "fatal: cannot clone empty repository"
 			if ln "$repo/$sample_file" "$GIT_DIR/objects/sample" 2>/dev/null
 			then
 				rm -f "$GIT_DIR/objects/sample"

-- 
1.5.3.7.2224.ge4a5

Jeff King [off-list ref] wrote:

git-config recently learned a --get-colorbool option. By
using it, we will get the same color=auto behavior that
other git commands have.

Specifically, this fixes the case where "color.diff = true"
meant "always" in git-svn, but "auto" in other programs.

Signed-off-by: Jeff King <redacted>

Acked-by: Eric Wong <redacted>

-- 
Eric Wong