[PATCH] quote_path: convert empty path to "./"

[PATCH] quote_path: convert empty path to "./"

[Jeff King]

Now that we are correctly removing leading prefixes from files in git
status, there is a degenerate case: the directory matching the prefix.
Because we show only the directory name for a directory that contains
only untracked files, it gets collapsed to an empty string.

Example:

  $ git init
  $ mkdir subdir
  $ touch subdir/file
  $ git status
  ...
  # Untracked files:
  #   (use "git add <file>..." to include in what will be committed)
  #
  #       subdir/

  So far, so good.

  $ cd subdir
  $ git status
  ....
  # Untracked files:
  #   (use "git add <file>..." to include in what will be committed)
  #
  #

  Oops, that's a bit confusing.

  This patch prints './' to show that there is some output.

---
I think it looks a bit ugly because it is so small (though just '.' was
even worse). But I don't see what else would make sense.

 wt-status.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 02dbb75..31d83bf 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -121,6 +121,9 @@ static char *quote_path(const char *in, int len,
 		}
 	}
 
+	if (!out->len)
+		strbuf_addstr(out, "./");
+
 	return out->buf;
 }
 
-- 
1.5.3.7.2156.g3d791-dirty

Re: [PATCH] quote_path: convert empty path to "./"

[Johannes Schindelin]

Hi,

On Fri, 7 Dec 2007, Jeff King wrote:

>   # Untracked files:
>   #   (use "git add <file>..." to include in what will be committed)
>   #
>   #       subdir/
> 
>   So far, so good.
> 
>   $ cd subdir
>   $ git status
>   ....
>   # Untracked files:
>   #   (use "git add <file>..." to include in what will be committed)
>   #
>   #
> 
>   Oops, that's a bit confusing.
> 
>   This patch prints './' to show that there is some output.

Sounds reasonable.

Ciao,
Dscho

Re: [PATCH] quote_path: convert empty path to "./"

[Thomas Harning]

Johannes Schindelin wrote:
> Hi,
>
> On Fri, 7 Dec 2007, Jeff King wrote:
>   
>> ...
>>     
>
> Sounds reasonable.
>
> Ciao,
> Dscho  
I concur.  There is one case that this seems to dodge.  What about the 
case where you are in:

/test/test_2  where /test  is not tracked...

This should probably show "./../"   not just "./"   , right?

Re: [PATCH] quote_path: convert empty path to "./"

[Jeff King]

On Fri, Dec 07, 2007 at 02:05:15PM -0500, Thomas Harning wrote:

> I concur.  There is one case that this seems to dodge.  What about the case 
> where you are in:
>
> /test/test_2  where /test  is not tracked...
>
> This should probably show "./../"   not just "./"   , right?

It already says "../", which is correct:

  $ git init
  $ mkdir test && cd test
  $ touch file
  $ mkdir test2 && cd test2
  $ git status
  ...
  # Untracked files:
  #   (use "git add <file>..." to include in what will be committed)
  #
  #       ../

There's no point in ever saying "./" _except_ in the case where the
output would be totally blank, since there is no way to tell that it is
an output line.

Personally, I don't like either the "../" or the "./", but I actually
think the relative paths are less readable than the full paths in
general.

-Peff

[PATCH] add status.relativePaths config variable

[Jeff King]

The output of git-status was recently changed to output
relative paths. Setting this variable to false restores the
old behavior for any old-timers that prefer it.

Signed-off-by: Jeff King <peff@peff.net>
---
On Fri, Dec 07, 2007 at 03:49:37PM -0500, Jeff King wrote:

> Personally, I don't like either the "../" or the "./", but I actually
> think the relative paths are less readable than the full paths in
> general.

So here is a config option to turn it off; I don't think there should be
any consistency problems, since git-status output is meant to be
human-readable (and after all, we just changed it :) ).

This patch also contains a small buglet fix in the neighboring code
where we didn't stop trying to match "color.status.*" even after we used
it to set the status color.

 Documentation/config.txt |    6 ++++++
 builtin-commit.c         |    3 +--
 builtin-revert.c         |    2 +-
 t/t7502-status.sh        |   31 +++++++++++++++++++++++++++++++
 wt-status.c              |   10 +++++++++-
 wt-status.h              |    2 +-
 6 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f0ffb9d..fabe7f8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -776,6 +776,12 @@ showbranch.default::
 	The default set of branches for gitlink:git-show-branch[1].
 	See gitlink:git-show-branch[1].
 
+status.relativePaths::
+	By default, gitlink:git-status[1] shows paths relative to the
+	current directory. Setting this variable to `false` shows paths
+	relative to the repository root (this was the default for git
+	prior to v1.5.4).
+
 tar.umask::
 	This variable can be used to restrict the permission bits of
 	tar archive entries.  The default is 0002, which turns off the
diff --git a/builtin-commit.c b/builtin-commit.c
index 18c6323..04b3bf1 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -284,8 +284,7 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix)
 {
 	struct wt_status s;
 
-	wt_status_prepare(&s);
-	s.prefix = prefix;
+	wt_status_prepare(&s, prefix);
 
 	if (amend) {
 		s.amend = 1;
diff --git a/builtin-revert.c b/builtin-revert.c
index 4bf8eb2..c285f8e 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -277,7 +277,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 
 		if (get_sha1("HEAD", head))
 			die ("You do not have a valid HEAD");
-		wt_status_prepare(&s);
+		wt_status_prepare(&s, NULL);
 		if (s.commitable)
 			die ("Dirty index: cannot %s", me);
 		discard_cache();
diff --git a/t/t7502-status.sh b/t/t7502-status.sh
index d6ae69d..9ce50ca 100755
--- a/t/t7502-status.sh
+++ b/t/t7502-status.sh
@@ -88,4 +88,35 @@ test_expect_success 'status with relative paths' '
 
 '
 
+cat > expect << \EOF
+# On branch master
+# Changes to be committed:
+#   (use "git reset HEAD <file>..." to unstage)
+#
+#	new file:   dir2/added
+#
+# Changed but not updated:
+#   (use "git add <file>..." to update what will be committed)
+#
+#	modified:   dir1/modified
+#
+# Untracked files:
+#   (use "git add <file>..." to include in what will be committed)
+#
+#	dir1/untracked
+#	dir2/modified
+#	dir2/untracked
+#	expect
+#	output
+#	untracked
+EOF
+
+test_expect_success 'status without relative paths' '
+
+	git config status.relativePaths false
+	(cd dir1 && git status) > output &&
+	git diff expect output
+
+'
+
 test_done
diff --git a/wt-status.c b/wt-status.c
index 02dbb75..b21b2c4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -8,6 +8,7 @@
 #include "revision.h"
 #include "diffcore.h"
 
+int wt_status_relative_paths = 1;
 int wt_status_use_color = 0;
 static char wt_status_colors[][COLOR_MAXLEN] = {
 	"",         /* WT_STATUS_HEADER: normal */
@@ -42,7 +43,7 @@ static const char* color(int slot)
 	return wt_status_use_color ? wt_status_colors[slot] : "";
 }
 
-void wt_status_prepare(struct wt_status *s)
+void wt_status_prepare(struct wt_status *s, const char *prefix)
 {
 	unsigned char sha1[20];
 	const char *head;
@@ -53,6 +54,8 @@ void wt_status_prepare(struct wt_status *s)
 	s->reference = "HEAD";
 	s->fp = stdout;
 	s->index_file = get_index_file();
+	if (wt_status_relative_paths)
+		s->prefix = prefix;
 }
 
 static void wt_status_print_cached_header(struct wt_status *s)
@@ -397,6 +400,11 @@ int git_status_config(const char *k, const char *v)
 	if (!prefixcmp(k, "status.color.") || !prefixcmp(k, "color.status.")) {
 		int slot = parse_status_slot(k, 13);
 		color_parse(v, k, wt_status_colors[slot]);
+		return 0;
+	}
+	if (!strcmp(k, "status.relativepaths")) {
+		wt_status_relative_paths = git_config_bool(k, v);
+		return 0;
 	}
 	return git_default_config(k, v);
 }
diff --git a/wt-status.h b/wt-status.h
index 225fb4d..0ed94f3 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -28,7 +28,7 @@ struct wt_status {
 
 int git_status_config(const char *var, const char *value);
 int wt_status_use_color;
-void wt_status_prepare(struct wt_status *s);
+void wt_status_prepare(struct wt_status *s, const char *prefix);
 void wt_status_print(struct wt_status *s);
 
 #endif /* STATUS_H */
-- 
1.5.3.7.2159.gde63a-dirty

Re: [PATCH] add status.relativePaths config variable

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The output of git-status was recently changed to output
> relative paths. Setting this variable to false restores the
> old behavior for any old-timers that prefer it.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> On Fri, Dec 07, 2007 at 03:49:37PM -0500, Jeff King wrote:
>
>> Personally, I don't like either the "../" or the "./", but I actually
>> think the relative paths are less readable than the full paths in
>> general.
>
> So here is a config option to turn it off; I don't think there should be
> any consistency problems, since git-status output is meant to be
> human-readable (and after all, we just changed it :) ).

I like the general idea (and suspect we might want to make it default to
false to retain the original behaviour, but I'd refrain from suggesting
it, to keep the user experience stable during the upcoming -rc period).

We'd need an update to git-status documentation to mention the variable.

> diff --git a/builtin-commit.c b/builtin-commit.c
> index 18c6323..04b3bf1 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -284,8 +284,7 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix)
>  {
>  	struct wt_status s;
>  
> -	wt_status_prepare(&s);
> -	s.prefix = prefix;
> +	wt_status_prepare(&s, prefix);

I have been wondering ever since receiving this patch if this is a good
interface change.  Was there a problem if instead:

	- The implementation of wt_status_prepare(&s) stays as before;

	- run_status(), after calling wt_status_prepare(&s), notices the
          configuration variable, and sets s.prefix conditionally;

Re: [PATCH] add status.relativePaths config variable

[Junio C Hamano]

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

> I have been wondering ever since receiving this patch if this is a good
> interface change.  Was there a problem if instead:
>
> 	- The implementation of wt_status_prepare(&s) stays as before;
>
> 	- run_status(), after calling wt_status_prepare(&s), notices the
>           configuration variable, and sets s.prefix conditionally;

Which would make the rewritten patch like this...

-- >8 --
From: Jeff King <peff@peff.net>
Date: Fri, 7 Dec 2007 16:26:07 -0500
Subject: [PATCH] add status.relativePaths config variable

The output of git-status was recently changed to output relative
paths. Setting this variable to false restores the old behavior for
any old-timers that prefer it.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt     |    6 ++++++
 Documentation/git-status.txt |    5 ++++-
 builtin-commit.c             |    3 ++-
 t/t7502-status.sh            |   31 +++++++++++++++++++++++++++++++
 wt-status.c                  |    6 ++++++
 wt-status.h                  |    1 +
 6 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 736fcd7..79d51f2 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -762,6 +762,12 @@ showbranch.default::
 	The default set of branches for gitlink:git-show-branch[1].
 	See gitlink:git-show-branch[1].
 
+status.relativePaths::
+	By default, gitlink:git-status[1] shows paths relative to the
+	current directory. Setting this variable to `false` shows paths
+	relative to the repository root (this was the default for git
+	prior to v1.5.4).
+
 tar.umask::
 	This variable can be used to restrict the permission bits of
 	tar archive entries.  The default is 0002, which turns off the
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index b0cb6bc..bd4d787 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -42,7 +42,10 @@ template comments, and all the output lines are prefixed with '#'.
 
 The paths mentioned in the output, unlike many other git commands, are
 made relative to the current directory, if you are working in a
-subdirectory (this is on purpose, to help cutting and pasting).
+subdirectory (this is on purpose, to help cutting and pasting).  You can
+restore the older behaviour of showing the paths as relative to the top
+of the work tree by setting `status.relativepaths` configuration
+variable to `false`.
 
 
 CONFIGURATION
diff --git a/builtin-commit.c b/builtin-commit.c
index 2ec8223..19297ac 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -285,7 +285,8 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix)
 	struct wt_status s;
 
 	wt_status_prepare(&s);
-	s.prefix = prefix;
+	if (wt_status_relative_paths)
+		s.prefix = prefix;
 
 	if (amend) {
 		s.amend = 1;
diff --git a/t/t7502-status.sh b/t/t7502-status.sh
index d6ae69d..9ce50ca 100755
--- a/t/t7502-status.sh
+++ b/t/t7502-status.sh
@@ -88,4 +88,35 @@ test_expect_success 'status with relative paths' '
 
 '
 
+cat > expect << \EOF
+# On branch master
+# Changes to be committed:
+#   (use "git reset HEAD <file>..." to unstage)
+#
+#	new file:   dir2/added
+#
+# Changed but not updated:
+#   (use "git add <file>..." to update what will be committed)
+#
+#	modified:   dir1/modified
+#
+# Untracked files:
+#   (use "git add <file>..." to include in what will be committed)
+#
+#	dir1/untracked
+#	dir2/modified
+#	dir2/untracked
+#	expect
+#	output
+#	untracked
+EOF
+
+test_expect_success 'status without relative paths' '
+
+	git config status.relativePaths false
+	(cd dir1 && git status) > output &&
+	git diff expect output
+
+'
+
 test_done
diff --git a/wt-status.c b/wt-status.c
index 05414bb..51c1879 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -8,6 +8,7 @@
 #include "revision.h"
 #include "diffcore.h"
 
+int wt_status_relative_paths = 1;
 int wt_status_use_color = 0;
 static char wt_status_colors[][COLOR_MAXLEN] = {
 	"",         /* WT_STATUS_HEADER: normal */
@@ -400,6 +401,11 @@ int git_status_config(const char *k, const char *v)
 	if (!prefixcmp(k, "status.color.") || !prefixcmp(k, "color.status.")) {
 		int slot = parse_status_slot(k, 13);
 		color_parse(v, k, wt_status_colors[slot]);
+		return 0;
+	}
+	if (!strcmp(k, "status.relativepaths")) {
+		wt_status_relative_paths = git_config_bool(k, v);
+		return 0;
 	}
 	return git_default_config(k, v);
 }
diff --git a/wt-status.h b/wt-status.h
index 225fb4d..63d50f2 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -28,6 +28,7 @@ struct wt_status {
 
 int git_status_config(const char *var, const char *value);
 int wt_status_use_color;
+int wt_status_relative_paths;
 void wt_status_prepare(struct wt_status *s);
 void wt_status_print(struct wt_status *s);
 
-- 
1.5.3.7-2182-g108b

Re: [PATCH] add status.relativePaths config variable

[Jeff King]

On Fri, Dec 07, 2007 at 11:47:56PM -0800, Junio C Hamano wrote:

> Which would make the rewritten patch like this...

Looks like our patches just crossed paths.  Yours looks OK, but we
should add something to the 'configuration' section, and...

> diff --git a/wt-status.h b/wt-status.h
> index 225fb4d..63d50f2 100644
> --- a/wt-status.h
> +++ b/wt-status.h
> @@ -28,6 +28,7 @@ struct wt_status {
>  
>  int git_status_config(const char *var, const char *value);
>  int wt_status_use_color;
> +int wt_status_relative_paths;
>  void wt_status_prepare(struct wt_status *s);
>  void wt_status_print(struct wt_status *s);

Shouldn't both of these ints be marked "extern"? I'm surprised it worked
at all (or perhaps the part of my brain that stores C linkage issues is
rotting?).

-Peff

Re: [PATCH] add status.relativePaths config variable

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Dec 07, 2007 at 11:47:56PM -0800, Junio C Hamano wrote:
>
>> Which would make the rewritten patch like this...
>
> Looks like our patches just crossed paths.  Yours looks OK, but we
> should add something to the 'configuration' section, and...
>
>> diff --git a/wt-status.h b/wt-status.h
>> index 225fb4d..63d50f2 100644
>> --- a/wt-status.h
>> +++ b/wt-status.h
>> @@ -28,6 +28,7 @@ struct wt_status {
>>  
>>  int git_status_config(const char *var, const char *value);
>>  int wt_status_use_color;
>> +int wt_status_relative_paths;
>>  void wt_status_prepare(struct wt_status *s);
>>  void wt_status_print(struct wt_status *s);
>
> Shouldn't both of these ints be marked "extern"? I'm surprised it worked
> at all (or perhaps the part of my brain that stores C linkage issues is
> rotting?).

Yes, rotting very much.

Re: [PATCH] add status.relativePaths config variable

[Jeff King]

On Sat, Dec 08, 2007 at 12:05:49AM -0800, Junio C Hamano wrote:

> >> index 225fb4d..63d50f2 100644
> >> --- a/wt-status.h
> >> +++ b/wt-status.h
> >> @@ -28,6 +28,7 @@ struct wt_status {
> >>  
> >>  int git_status_config(const char *var, const char *value);
> >>  int wt_status_use_color;
> >> +int wt_status_relative_paths;
> >>  void wt_status_prepare(struct wt_status *s);
> >>  void wt_status_print(struct wt_status *s);
> >
> > Shouldn't both of these ints be marked "extern"? I'm surprised it worked
> > at all (or perhaps the part of my brain that stores C linkage issues is
> > rotting?).
> 
> Yes, rotting very much.

Nope, there's still a little grey matter left. It is not technically
guaranteed by the standard to work, since the declaration in every
source file which includes wt-status.h is a "tentative definition."
Fortunately, the linker is nice enough to figure out what's going on as
long as only one is actually initialized. This is listed in C99 Section
J.5.11 as a "Common extension".

The Summit C FAQ mentions it as well:

  http://c-faq.com/decl/decldef.html

So "extern" is better, but apparently not required for any
linkers we care about. Note that omitting "extern" _is_ illegal in C++,
but fortunately we _really_ don't care about those linkers. :)

But at least I'm not totally crazy.

-Peff

Re: [PATCH] add status.relativePaths config variable

[Jeff King]

On Fri, Dec 07, 2007 at 11:34:14PM -0800, Junio C Hamano wrote:

> I like the general idea (and suspect we might want to make it default to
> false to retain the original behaviour, but I'd refrain from suggesting
> it, to keep the user experience stable during the upcoming -rc period).
> 
> We'd need an update to git-status documentation to mention the variable.

Patch is below; please squash it into the original.

It seems kind of silly to manually write the "Configuration" section for
git-status, though. It would be nice if our config.txt could be
annotated to mention which commands use which config variables, and
git-*.txt could automagically include the right sections.

> > -	wt_status_prepare(&s);
> > -	s.prefix = prefix;
> > +	wt_status_prepare(&s, prefix);
> 
> I have been wondering ever since receiving this patch if this is a good
> interface change.  Was there a problem if instead:
> 
> 	- The implementation of wt_status_prepare(&s) stays as before;
> 
> 	- run_status(), after calling wt_status_prepare(&s), notices the
>           configuration variable, and sets s.prefix conditionally;

That would work fine. My reasoning was: the point of wt_status_prepare
is to initialize the wt_status object. I thought the "whether to use
relative paths based on config" logic should be something that _every_
preparer uses. OTOH, when I wrote it, I never expected that anyone _but_
run_status would call it (I must confess to not really investigating why
git-revert needed it; looks like it is to find a dirty index or working
tree, which is a little silly, since as a side effect we will do a find
on all untracked files).

I am fine with either; your call. Documentation patch is below.

---
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index b0cb6bc..645dc85 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -42,7 +42,8 @@ template comments, and all the output lines are prefixed with '#'.
 
 The paths mentioned in the output, unlike many other git commands, are
 made relative to the current directory, if you are working in a
-subdirectory (this is on purpose, to help cutting and pasting).
+subdirectory (this is on purpose, to help cutting and pasting). See
+the status.relativePaths config option below.
 
 
 CONFIGURATION
@@ -53,6 +54,10 @@ mean the same thing and the latter is kept for backward
 compatibility) and `color.status.<slot>` configuration variables
 to colorize its output.
 
+If the config variable `status.relativePaths` is set to false, then all
+paths shown are relative to the repository root, not to the current
+directory.
+
 See Also
 --------
 gitlink:gitignore[5]

Re: [PATCH] add status.relativePaths config variable

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Dec 07, 2007 at 11:34:14PM -0800, Junio C Hamano wrote:
> ...
>> I have been wondering ever since receiving this patch if this is a good
>> interface change.  Was there a problem if instead:
>> 
>> 	- The implementation of wt_status_prepare(&s) stays as before;
>> 
>> 	- run_status(), after calling wt_status_prepare(&s), notices the
>>           configuration variable, and sets s.prefix conditionally;
>
> That would work fine. My reasoning was: the point of wt_status_prepare
> is to initialize the wt_status object.

Yes, just like diffopts and revs.  They initialize the object to a plain
vanilla defaults, and the caller uses other methods (either direct
assignments to members or by calling helper functions such as
diff_opt_parse() and setup_revisions()) to fill in specialized values.

And s.prefix is very much special case.  That's the reasoning behind my
suggestion.

> ..., which is a little silly, since as a side effect we will do a find
> on all untracked files).

Ah, that is probably the side effect of direct rewrite from shell script
to C.  We should drop that and replace with what Alex did recently to
git-commit --no-edit codepath.

> I am fine with either; your call. Documentation patch is below.

Thanks.  Will take it.