threads / patch / 7967

patchSupport ent:relative_path

Subject: [PATCH] Support ent:relative_path

## tl;dr

28 messages between May 4, 2007 and May 5, 2007. Diffs are folded; open one to read it.

replies: 27people: 6as markdown or json

Dana How· May 4, 2007, 07:18 UTC · lore
Most commands accept relative paths,  but this is
not true of arguments in ent:path format.  This
patch makes all 3 of the following git-show commands
work in the git source tree (not just the first):
 % cd xdiff
 % git-show v1.5.2-rc0:xdiff/xemit.h
 % git-show v1.5.2-rc0:./xemit.h
 % git-config --bool core.relativepaths yes
 % git-show v1.5.2-rc0:xemit.h
Signed-off-by: Dana L. How <danahow@gmail.com>
---
 cache.h       |    2 ++
 config.c      |    5 +++++
 environment.c |    1 +
 setup.c       |    5 ++++-
 sha1_name.c   |   27 ++++++++++++++++++++++++---
 5 files changed, 36 insertions(+), 4 deletions(-)
Show changes to 5 files +36 −4

cache.h, config.c, environment.c, setup.c, sha1_name.c

diff --git a/cache.h b/cache.h
index 8e76152..fc3fcb1 100644
--- a/cache.h
+++ b/cache.h
@@ -215,6 +215,7 @@ extern char *get_graft_file(void);
 
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
+extern const char *prefix_to_cwd;
 extern const char **get_pathspec(const char *prefix, const char **pathspec);
 extern const char *setup_git_directory_gently(int *);
 extern const char *setup_git_directory(void);
@@ -276,6 +277,7 @@ extern int delete_ref(const char *, const unsigned char *sha1);
 extern int use_legacy_headers;
 extern int trust_executable_bit;
 extern int has_symlinks;
+extern int assume_relative_paths;
 extern int assume_unchanged;
 extern int prefer_symlink_refs;
 extern int log_all_ref_updates;
diff --git a/config.c b/config.c
index 70d1055..7525965 100644
--- a/config.c
+++ b/config.c
@@ -279,6 +279,11 @@ int git_default_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.relativepaths")) {
+		assume_relative_paths = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "core.ignorestat")) {
 		assume_unchanged = git_config_bool(var, value);
 		return 0;
diff --git a/environment.c b/environment.c
index 2231659..f1b867d 100644
--- a/environment.c
+++ b/environment.c
@@ -14,6 +14,7 @@ char git_default_name[MAX_GITNAME];
 int use_legacy_headers = 1;
 int trust_executable_bit = 1;
 int has_symlinks = 1;
+int assume_relative_paths;
 int assume_unchanged;
 int prefer_symlink_refs;
 int is_bare_repository_cfg = -1; /* unspecified */
diff --git a/setup.c b/setup.c
index a45ea83..46ae6e3 100644
--- a/setup.c
+++ b/setup.c
@@ -1,5 +1,7 @@
 #include "cache.h"
 
+const char *prefix_to_cwd;
+
 const char *prefix_path(const char *prefix, int len, const char *path)
 {
 	const char *orig = path;
@@ -252,7 +254,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	cwd[len++] = '/';
 	cwd[len] = 0;
 	inside_git_dir = !prefixcmp(cwd + offset, ".git/");
-	return cwd + offset;
+	prefix_to_cwd = cwd + offset;
+	return prefix_to_cwd;
 }
 
 int git_config_perm(const char *var, const char *value)
diff --git a/sha1_name.c b/sha1_name.c
index 55f25a2..0b9e92c 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -592,6 +592,24 @@ static int handle_one_ref(const char *path,
 	return 0;
 }
 
+static void prepend_prefix(const char **cp, int *namelen)
+{
+	static char fullpath[PATH_MAX];
+	if (*namelen > 2 && !memcmp(*cp, "./", 2)) {
+		*cp += 2;
+		*namelen -= 2;
+	} else
+	if (!assume_relative_paths || !prefix_to_cwd)
+		return;
+
+	*namelen += strlen(prefix_to_cwd);
+	if (*namelen >= PATH_MAX)
+		die("path too long");
+	strcpy(fullpath, prefix_to_cwd);
+	strcat(fullpath, *cp);
+	*cp = fullpath;
+}
+
 /*
  * This interprets names like ':/Initial revision of "git"' by searching
  * through history and returning the first commit whose message starts
@@ -681,6 +699,7 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
 			read_cache();
 		if (active_nr < 0)
 			return -1;
+		prepend_prefix(&cp, &namelen);
 		pos = cache_name_pos(cp, namelen);
 		if (pos < 0)
 			pos = -pos - 1;
@@ -708,9 +727,11 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
 	}
 	if (*cp == ':') {
 		unsigned char tree_sha1[20];
-		if (!get_sha1_1(name, cp-name, tree_sha1))
-			return get_tree_entry(tree_sha1, cp+1, sha1,
-					      mode);
+		if (!get_sha1_1(name, cp - name, tree_sha1)) {
+			namelen -= ++cp - name;
+			prepend_prefix(&cp, &namelen);
+			return get_tree_entry(tree_sha1, cp, sha1, mode);
+		}
 	}
 	return ret;
 }
-- 
1.5.2.rc0.787.g0014
Dana How· May 4, 2007, 07:22 UTC · re: Dana How · lore

Re: [PATCH] Support ent:relative_path

Yikes... the if's in prepend_prefix() need to be re-arranged.
Dana
On 5/4/07, Dana How <danahow@gmail.com> wrote:
Show 146 quoted lines
>
> Most commands accept relative paths,  but this is
> not true of arguments in ent:path format.  This
> patch makes all 3 of the following git-show commands
> work in the git source tree (not just the first):
>  % cd xdiff
>  % git-show v1.5.2-rc0:xdiff/xemit.h
>  % git-show v1.5.2-rc0:./xemit.h
>  % git-config --bool core.relativepaths yes
>  % git-show v1.5.2-rc0:xemit.h
>
> Signed-off-by: Dana L. How <danahow@gmail.com>
> ---
>  cache.h       |    2 ++
>  config.c      |    5 +++++
>  environment.c |    1 +
>  setup.c       |    5 ++++-
>  sha1_name.c   |   27 ++++++++++++++++++++++++---
>  5 files changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 8e76152..fc3fcb1 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -215,6 +215,7 @@ extern char *get_graft_file(void);
>
>  #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
>
> +extern const char *prefix_to_cwd;
>  extern const char **get_pathspec(const char *prefix, const char **pathspec);
>  extern const char *setup_git_directory_gently(int *);
>  extern const char *setup_git_directory(void);
> @@ -276,6 +277,7 @@ extern int delete_ref(const char *, const unsigned char *sha1);
>  extern int use_legacy_headers;
>  extern int trust_executable_bit;
>  extern int has_symlinks;
> +extern int assume_relative_paths;
>  extern int assume_unchanged;
>  extern int prefer_symlink_refs;
>  extern int log_all_ref_updates;
> diff --git a/config.c b/config.c
> index 70d1055..7525965 100644
> --- a/config.c
> +++ b/config.c
> @@ -279,6 +279,11 @@ int git_default_config(const char *var, const char *value)
>                 return 0;
>         }
>
> +       if (!strcmp(var, "core.relativepaths")) {
> +               assume_relative_paths = git_config_bool(var, value);
> +               return 0;
> +       }
> +
>         if (!strcmp(var, "core.ignorestat")) {
>                 assume_unchanged = git_config_bool(var, value);
>                 return 0;
> diff --git a/environment.c b/environment.c
> index 2231659..f1b867d 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -14,6 +14,7 @@ char git_default_name[MAX_GITNAME];
>  int use_legacy_headers = 1;
>  int trust_executable_bit = 1;
>  int has_symlinks = 1;
> +int assume_relative_paths;
>  int assume_unchanged;
>  int prefer_symlink_refs;
>  int is_bare_repository_cfg = -1; /* unspecified */
> diff --git a/setup.c b/setup.c
> index a45ea83..46ae6e3 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1,5 +1,7 @@
>  #include "cache.h"
>
> +const char *prefix_to_cwd;
> +
>  const char *prefix_path(const char *prefix, int len, const char *path)
>  {
>         const char *orig = path;
> @@ -252,7 +254,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
>         cwd[len++] = '/';
>         cwd[len] = 0;
>         inside_git_dir = !prefixcmp(cwd + offset, ".git/");
> -       return cwd + offset;
> +       prefix_to_cwd = cwd + offset;
> +       return prefix_to_cwd;
>  }
>
>  int git_config_perm(const char *var, const char *value)
> diff --git a/sha1_name.c b/sha1_name.c
> index 55f25a2..0b9e92c 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -592,6 +592,24 @@ static int handle_one_ref(const char *path,
>         return 0;
>  }
>
> +static void prepend_prefix(const char **cp, int *namelen)
> +{
> +       static char fullpath[PATH_MAX];
> +       if (*namelen > 2 && !memcmp(*cp, "./", 2)) {
> +               *cp += 2;
> +               *namelen -= 2;
> +       } else
> +       if (!assume_relative_paths || !prefix_to_cwd)
> +               return;
> +
> +       *namelen += strlen(prefix_to_cwd);
> +       if (*namelen >= PATH_MAX)
> +               die("path too long");
> +       strcpy(fullpath, prefix_to_cwd);
> +       strcat(fullpath, *cp);
> +       *cp = fullpath;
> +}
> +
>  /*
>   * This interprets names like ':/Initial revision of "git"' by searching
>   * through history and returning the first commit whose message starts
> @@ -681,6 +699,7 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
>                         read_cache();
>                 if (active_nr < 0)
>                         return -1;
> +               prepend_prefix(&cp, &namelen);
>                 pos = cache_name_pos(cp, namelen);
>                 if (pos < 0)
>                         pos = -pos - 1;
> @@ -708,9 +727,11 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
>         }
>         if (*cp == ':') {
>                 unsigned char tree_sha1[20];
> -               if (!get_sha1_1(name, cp-name, tree_sha1))
> -                       return get_tree_entry(tree_sha1, cp+1, sha1,
> -                                             mode);
> +               if (!get_sha1_1(name, cp - name, tree_sha1)) {
> +                       namelen -= ++cp - name;
> +                       prepend_prefix(&cp, &namelen);
> +                       return get_tree_entry(tree_sha1, cp, sha1, mode);
> +               }
>         }
>         return ret;
>  }
> --
> 1.5.2.rc0.787.g0014
>
>
-- 
Dana L. How  danahow@gmail.com  +1 650 804 5991 cell
Junio C Hamano· May 4, 2007, 08:21 UTC · re: Dana How · lore

Re: [PATCH] Support ent:relative_path

I do not think it is wrong per-se, to want to make this hold true:
	A=$(git rev-parse :somedir/file)
	B=$(cd somedir && git rev-parse :file)
        test "$A" = "$B"

One thing I am reasonably certain however is that this should NOT be conditional to a config setting. Doing so would force scripts that take (or compute) path and commit and concatenate them to make "${commit}:${path}" to name a blob (or tree) to first inspect the current setting of core.relativepaths and undo what the new code does by prefixing/subtracting the prefix string depending on the config.

In other words, having that config is not really helping scripts or compatibility.

I think the choices are:
 (1) we say it was a mistake that we did not make it relative to
     the current directory when we introduced the X:<path>
     syntax (X could be empty or :[0-3]: for index, or a commit
     or tree object name), and change the semantics in a future
     major release for everybody, apologizing for potentially
     breaking existing scripts; or
 (2) keep the current behaviour as is, and come up with a
     different syntax to use relative; or
 (3) do nothing.

My preference is (2), (3) and then (1), but I do not have offhand a suggestion for a good metacharacter we could use.

Dana How· May 4, 2007, 08:45 UTC · re: Junio C Hamano · lore

Re: [PATCH] Support ent:relative_path

On 5/4/07, Junio C Hamano <junkio@cox.net> wrote:
Show 33 quoted lines
> I do not think it is wrong per-se, to want to make this hold true:
>
>         A=$(git rev-parse :somedir/file)
>         B=$(cd somedir && git rev-parse :file)
>         test "$A" = "$B"
>
> One thing I am reasonably certain however is that this should
> NOT be conditional to a config setting.  Doing so would force
> scripts that take (or compute) path and commit and concatenate
> them to make "${commit}:${path}" to name a blob (or tree) to
> first inspect the current setting of core.relativepaths and undo
> what the new code does by prefixing/subtracting the prefix
> string depending on the config.
>
> In other words, having that config is not really helping scripts
> or compatibility.
>
> I think the choices are:
>
>  (1) we say it was a mistake that we did not make it relative to
>      the current directory when we introduced the X:<path>
>      syntax (X could be empty or :[0-3]: for index, or a commit
>      or tree object name), and change the semantics in a future
>      major release for everybody, apologizing for potentially
>      breaking existing scripts; or
>
>  (2) keep the current behaviour as is, and come up with a
>      different syntax to use relative; or
>
>  (3) do nothing.
>
> My preference is (2), (3) and then (1), but I do not have
> offhand a suggestion for a good metacharacter we could use.
Let us require:
* No config settings as you prefer
* Current interpretations are unchanged.
The current path supports
  :fullpath
  :/string
  :./relpath
[assuming core.relativepaths=no always since it goes away].
(:/string is the "find string in commit msg" feature.)
A new metacharacter lets us drop the "./" ,
and I think that's desirable.
How about = ? It's not even shifted on my keyboard:
 ent=relpath
 ent=2=relpath
 ent=/fullpath
1. We would accept the same new metacharacter before stage if any.
2, You could still use ent:/string , or ent:fullpath .

The patch should also be extended so relpath can start with a sequence of ./ or ../ prefixes, which is easily handled in prepend_prefix()..

What do you think of "=" ?
-- 
Dana L. How  danahow@gmail.com  +1 650 804 5991 cell
Alex Riesen· May 4, 2007, 08:47 UTC · re: Junio C Hamano · lore

Re: [PATCH] Support ent:relative_path

On 5/4/07, Junio C Hamano <junkio@cox.net> wrote:
Show 6 quoted lines
>  (1) we say it was a mistake that we did not make it relative to
>      the current directory when we introduced the X:<path>
>      syntax (X could be empty or :[0-3]: for index, or a commit
>      or tree object name), and change the semantics in a future
>      major release for everybody, apologizing for potentially
>      breaking existing scripts; or
That would be my first prio preference
Show 7 quoted lines
>  (2) keep the current behaviour as is, and come up with a
>      different syntax to use relative; or
>
>  (3) do nothing.
>
> My preference is (2), (3) and then (1), but I do not have
> offhand a suggestion for a good metacharacter we could use.
"./" :)
Dana How· May 4, 2007, 08:53 UTC · re: Alex Riesen · lore

Re: [PATCH] Support ent:relative_path

On 5/4/07, Alex Riesen <raa.lkml@gmail.com> wrote:
Show 9 quoted lines
> On 5/4/07, Junio C Hamano <junkio@cox.net> wrote:
> >  (1) we say it was a mistake that we did not make it relative to
> >      the current directory when we introduced the X:<path>
> >      syntax (X could be empty or :[0-3]: for index, or a commit
> >      or tree object name), and change the semantics in a future
> >      major release for everybody, apologizing for potentially
> >      breaking existing scripts; or
>
> That would be my first prio preference
Mine too,  but I don't want to break anything.
Show 9 quoted lines
> >  (2) keep the current behaviour as is, and come up with a
> >      different syntax to use relative; or
> >
> >  (3) do nothing.
> >
> > My preference is (2), (3) and then (1), but I do not have
> > offhand a suggestion for a good metacharacter we could use.
>
> "./" :)
Heh.  Yes, that works in the current patch.

I'm really starting to like the idea of introducing "=" as an alternative for ":" : "=" assumes relative paths, ":" assumes absolute (full) paths. That could be Junio's new metacharacter.

Since "=" is slightly easier to type [ ;-) ], and looks like a stretched ":", it's almost ingenious... (Definitely getting carried away!)

-- 
Dana L. How  danahow@gmail.com  +1 650 804 5991 cell
Alex Riesen· May 4, 2007, 09:17 UTC · re: Dana How · lore

Re: [PATCH] Support ent:relative_path

On 5/4/07, Dana How <danahow@gmail.com> wrote:
Show 10 quoted lines
> > >  (1) we say it was a mistake that we did not make it relative to
> > >      the current directory when we introduced the X:<path>
> > >      syntax (X could be empty or :[0-3]: for index, or a commit
> > >      or tree object name), and change the semantics in a future
> > >      major release for everybody, apologizing for potentially
> > >      breaking existing scripts; or
> >
> > That would be my first prio preference
>
> Mine too,  but I don't want to break anything.

It probably wont break anything which is not already broken. It's somehow hard to imagine someone would use the syntax to check for absence of a file in non-top-level subdirectory. Besides, the existing syntax is ambiguous: for anyone using pathname tab completion.

Besides, you can give a warning of ambiguity in case there are equal relative and a top-level paths. Return the relative, but print the warning, so people can fix their scripts.

Show 10 quoted lines
> > > My preference is (2), (3) and then (1), but I do not have
> > > offhand a suggestion for a good metacharacter we could use.
> >
> > "./" :)
> Heh.  Yes, that works in the current patch.
>
> I'm really starting to like the idea of introducing "="
> as an alternative for ":" :
> "=" assumes relative paths, ":" assumes absolute (full) paths.
> That could be Junio's new metacharacter.

Just keep in mind: "once introduced, you'll never be able to remove it". And I don't share your feelings regarding the special character, for instance.

I'd suggest to define a special character for _top_ level. Real pity ":/" is taken.

Dana How· May 4, 2007, 09:26 UTC · re: Alex Riesen · lore

Re: [PATCH] Support ent:relative_path

On 5/4/07, Alex Riesen <raa.lkml@gmail.com> wrote:
Show 13 quoted lines
> > > > My preference is (2), (3) and then (1), but I do not have
> > > > offhand a suggestion for a good metacharacter we could use.
> > >
> > > "./" :)
> > Heh.  Yes, that works in the current patch.
> >
> > I'm really starting to like the idea of introducing "="
> > as an alternative for ":" :
> > "=" assumes relative paths, ":" assumes absolute (full) paths.
> > That could be Junio's new metacharacter.
>
> Just keep in mind: "once introduced, you'll never be able to remove it".
> And I don't share your feelings regarding the special character, for instance.
Indeed.
> I'd suggest to define a special character for _top_ level. Real pity
> ":/" is taken.

We could use ://fullpath for top level, and :relpath for relative. Then "string" in :/string couldn't start with /, which shouldn't be a problem (right?). I've certainly seen double slashes before; perforce in fact uses them for the root of the repository (depot).

This all depends on deciding that :relpath should be the (incompatible) new default, and I'm not sure that's going to happen.

-- 
Dana L. How  danahow@gmail.com  +1 650 804 5991 cell
Alex Riesen· May 4, 2007, 09:46 UTC · re: Dana How · lore

Re: [PATCH] Support ent:relative_path

On 5/4/07, Dana How <danahow@gmail.com> wrote:
> > I'd suggest to define a special character for _top_ level. Real pity
> > ":/" is taken.
> We could use ://fullpath for top level,

No good. How'd you find a commit starting with "/" than? (without changing ":/" syntax).

> and :relpath for relative. Then "string" in :/string couldn't start with /,
> which shouldn't be a problem (right?).  I've certainly seen double
> slashes before;
> perforce in fact uses them for the root of the repository (depot).

And I really hate perforce for its stupid redundancy (and changing of meaning of well-known idioms: why should // be anything special but plain top level or root?! Why the hell do they need them at if you cannot use relative paths in client specs at all?! Why can't the p4 command-line tool figure the fact from context or request the context be provided by user?! IOW, Perforce is a real bad example of how you do version control).

> This all depends on deciding that :relpath should be the (incompatible)
> new default, and I'm not sure that's going to happen.

If we are to stay that compatible, maybe ":./" for relative paths and the old syntax left to mean top-level would the best choice for now.

Dana How· May 4, 2007, 16:57 UTC · re: Alex Riesen · lore

Re: [PATCH] Support ent:relative_path

On 5/4/07, Alex Riesen <raa.lkml@gmail.com> wrote:
Show 7 quoted lines
> On 5/4/07, Dana How <danahow@gmail.com> wrote:
> > > I'd suggest to define a special character for _top_ level. Real pity
> > > ":/" is taken.
> > We could use ://fullpath for top level,
>
> No good. How'd you find a commit starting with "/" than? (without
> changing ":/" syntax).

Oh blechh. get_sha1_oneline uses prefixcmp(), not strstr(). Are there are any commits in git or the kernel starting with "/" ?

Show 12 quoted lines
> > and :relpath for relative. Then "string" in :/string couldn't start with /,
> > which shouldn't be a problem (right?).  I've certainly seen double
> > slashes before;
> > perforce in fact uses them for the root of the repository (depot).
>
> And I really hate perforce for its stupid redundancy (and changing of
> meaning of well-known idioms: why should // be anything special
> but plain top level or root?! Why the hell do they need them at if
> you cannot use relative paths in client specs at all?! Why can't the
> p4 command-line tool figure the fact from context or request the
> context be provided by user?! IOW, Perforce is a real bad example
> of how you do version control).

Whoa! I'm stuck using perforce too; I'm not holding it up as a *big* example. I originally saw // meaning root in the Apollo DOMAIN system, so for that reason it makes sense to me. I think it also means network root in Windoze (well, \\ does ;-) )..

Show 5 quoted lines
> > This all depends on deciding that :relpath should be the (incompatible)
> > new default, and I'm not sure that's going to happen.
>
> If we are to stay that compatible, maybe ":./" for relative paths and the
> old syntax left to mean top-level would the best choice for now.

Let's summarize so far: I think everyone's convinced me we need to be careful, so this email will be more tedious than I'd like.

(a) :./relpath clearly inidicates relative path. [Also take :../relpath .]
(b) I'd like a more natural way to do :./relpath (e.g. :relpath),
     or at least a future path to such.
(c) We would like to avoid new special characters beyond ":".
     This means everything has to be done with "." and "/".
(d) We are left with the following patterns:
     1. :string
     2. :/string
     3. ://string

[ We need a clear way to say relative, a clear way to say absolute, and the current :string can change from absolute to relative some time in the future if we so decide. ]

Ideas for (d) 2&3:
I. Make :/string actually match the RE ^[/]*string,  and ://string a full path.
   The leading [/]* is a very small change to get_sha1_oneline().
   [Or change prefixcmp() to strstr() in get_sha1_oneline().]
   How often do commit messages start with / ?
II. Make :/string a full path, and ://string match ^string .
   Is changing the current :/string to ://string less painful/dangerous?
III. Make :/string match ^string when string has no slashes,
    :/string a full path when string does have slashes,
    and ://string match ^string . Hmm,  seems confusing.
Do you use :/string now?  Since it's a case-sensitive exact match,
I don't think I'd even use it.
I find idea (II) most natural: absolute paths have one /,
and string matches have 2 suggesting an RE.

Concerning the current :string , perhaps we could do the following. There would be 2 internal fixed mode variables (NOT config variables) which do the following. The first controls whether this means an absolute or relative path. The second controls whether a warning message is printed whenever the first is consulted to make a decision. The interpretation of :string is left as-is, but motivated janitors (like me in this case) can use the second mode variable to change all :string patterns to :/string or ://string in scripts, letting us switch over later by changing one mode variable.

Someone mentioned DWIM for :string -- check both absolute and relative, in that order for compatibility probably. This seems a messy definition to me. Comments?

Anyway, this is more involved than I'd hoped, but it's good to think about consequences.

Thanks,
-- 
Dana L. How  danahow@gmail.com  +1 650 804 5991 cell
Alex Riesen· May 4, 2007, 17:17 UTC · re: Dana How · lore

Re: [PATCH] Support ent:relative_path

On 5/4/07, Dana How <danahow@gmail.com> wrote:
Show 10 quoted lines
> > > This all depends on deciding that :relpath should be the (incompatible)
> > > new default, and I'm not sure that's going to happen.
> >
> > If we are to stay that compatible, maybe ":./" for relative paths and the
> > old syntax left to mean top-level would the best choice for now.
>
> Let's summarize so far:  I think everyone's convinced me we need
> to be careful,  so this email will be more tedious than I'd like.
>
> (a) :./relpath clearly inidicates relative path. [Also take :../relpath .]
Ack
> (b) I'd like a more natural way to do :./relpath (e.g. :relpath),
>      or at least a future path to such.
> (c) We would like to avoid new special characters beyond ":".
>      This means everything has to be done with "." and "/".
And new semantics, if possible
> (d) We are left with the following patterns:
>      1. :string
Probably breaks something
>      2. :/string
Taken
>      3. ://string
Ugly (and breaks tab completion)
Show 9 quoted lines
> [ We need a clear way to say relative, a clear way to say absolute,
> and the current :string can change from absolute to relative some time
> in the future if we so decide. ]
>
> Ideas for (d) 2&3:
> I. Make :/string actually match the RE ^[/]*string,  and ://string a full path.
>    The leading [/]* is a very small change to get_sha1_oneline().
>    [Or change prefixcmp() to strstr() in get_sha1_oneline().]
>    How often do commit messages start with / ?
How often should they start to justify any decision?
> II. Make :/string a full path, and ://string match ^string .
>    Is changing the current :/string to ://string less painful/dangerous?
Johannes?
> III. Make :/string match ^string when string has no slashes,
>     :/string a full path when string does have slashes,
>     and ://string match ^string . Hmm,  seems confusing.
Very/
> Do you use :/string now?  Since it's a case-sensitive exact match,
No. It just exists, AFAICS
> I don't think I'd even use it.
> I find idea (II) most natural: absolute paths have one /,
> and string matches have 2 suggesting an RE.

I think, I'd be convinced by this one too, if not for "//" making no sense anywhere but root accidentally typed twice. I'd even sacrifice (or change) the search syntax for good.

Johannes Schindelin· May 4, 2007, 19:00 UTC · re: Alex Riesen · lore

Re: [PATCH] Support ent:relative_path

Hi,
On Fri, 4 May 2007, Alex Riesen wrote:
Show 6 quoted lines
> On 5/4/07, Dana How <danahow@gmail.com> wrote:
>
> > II. Make :/string a full path, and ://string match ^string .
> >    Is changing the current :/string to ://string less painful/dangerous?
> 
> Johannes?
Huh? Hello!
I did not follow this thread closely...

Having said that, I think ":./bla/blub" is the most intuitive, and the least breaking.

You _could_ change the current :/ notation, but _why_?

Ciao, Dscho

Alex Riesen· May 4, 2007, 20:23 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Support ent:relative_path

Johannes Schindelin, Fri, May 04, 2007 21:00:53 +0200:
> Having said that, I think ":./bla/blub" is the most intuitive, and the 
> least breaking.
> 
> You _could_ change the current :/ notation, but _why_?

To use the syntax for full path starting from root of repo. It looks "most intuitive" for this.

Johannes Sixt· May 4, 2007, 09:19 UTC · re: Alex Riesen · lore

Re: [PATCH] Support ent:relative_path

Alex Riesen wrote:
Show 5 quoted lines
> On 5/4/07, Junio C Hamano <junkio@cox.net> wrote:
> > My preference is (2), (3) and then (1), but I do not have
> > offhand a suggestion for a good metacharacter we could use.
> 
> "./" :)

+1, without the :) and ../ should DWIM, too.

-- Hannes
Junio C Hamano· May 4, 2007, 17:17 UTC · re: Johannes Sixt · lore

Re: [PATCH] Support ent:relative_path

Johannes Sixt <J.Sixt@eudaptics.com> writes:
Show 9 quoted lines
> Alex Riesen wrote:
>> On 5/4/07, Junio C Hamano <junkio@cox.net> wrote:
>> > My preference is (2), (3) and then (1), but I do not have
>> > offhand a suggestion for a good metacharacter we could use.
>> 
>> "./" :)
>
> +1, without the :)
> and ../ should DWIM, too.

I think these feel more or less natural (except that we do not say ":/fullpath" and instead say ":fullpath" which is unfortunate).

In the hindsight, if we had the perfect vision into the future, we would have made <path> relative to where you are when we initially did "<something>:<path>", with obvious semantics for things like "<something>:../<path>" and "<something>:/<path>".

We didn't. Further, we made a mistake to make :/ to mean something completely unrelated. My bad.

So I think "./" is the best compromise in the meantime.

With your suggestion, we can train people's fingers to type "./" now, and perhaps later in one of those big feature release like the 1.5.0 was, we could switch to "default to relative".

At the same time of that big "UI correction", we could make "<something>:/<path>" to mean "full path in commit (or index/stage) no matter where I am".

The current ":/<string>" is about going back, looking for the string, so it should not have used '/'; instead it should have been "<something>:?<string>". Maybe we could fix it by start accepting ":?" now (in addition to ":/"), give a big fat warning about ":/" going to mean a different thing, and encouraging users to use the question-mark form, in preparation for the big "UI correction".

Do people like that plan?
(soon after 1.5.2)
 - start accepting "<something>:./<path>" as "relative to where I am".
 - start accepting "<something>:?<string>" as "look back to find
   the string".
 - clearly explain the plan and prepare the users.
(a big release in the future, perhaps 1.6.0)
 - "<something>:<path>" becomes relative to where you are.
 - stop "<something>:/<string>" and start "<something>:/<fullpath>".
Dana How· May 4, 2007, 18:23 UTC · re: Junio C Hamano · lore

Re: [PATCH] Support ent:relative_path

On 5/4/07, Junio C Hamano <junkio@cox.net> wrote:
Show 57 quoted lines
> Johannes Sixt <J.Sixt@eudaptics.com> writes:
>
> > Alex Riesen wrote:
> >> On 5/4/07, Junio C Hamano <junkio@cox.net> wrote:
> >> > My preference is (2), (3) and then (1), but I do not have
> >> > offhand a suggestion for a good metacharacter we could use.
> >>
> >> "./" :)
> >
> > +1, without the :)
> > and ../ should DWIM, too.
>
> I think these feel more or less natural (except that we do not
> say ":/fullpath" and instead say ":fullpath" which is
> unfortunate).
>
> In the hindsight, if we had the perfect vision into the future,
> we would have made <path> relative to where you are when we
> initially did "<something>:<path>", with obvious semantics for
> things like "<something>:../<path>" and "<something>:/<path>".
>
> We didn't.  Further, we made a mistake to make :/ to mean
> something completely unrelated.  My bad.
>
> So I think "./" is the best compromise in the meantime.
>
> With your suggestion, we can train people's fingers to type "./"
> now, and perhaps later in one of those big feature release like
> the 1.5.0 was, we could switch to "default to relative".
>
> At the same time of that big "UI correction", we could make
> "<something>:/<path>" to mean "full path in commit (or
> index/stage) no matter where I am".
>
> The current ":/<string>" is about going back, looking for the
> string, so it should not have used '/'; instead it should have
> been "<something>:?<string>".  Maybe we could fix it by start
> accepting ":?" now (in addition to ":/"), give a big fat warning
> about ":/" going to mean a different thing, and encouraging
> users to use the question-mark form, in preparation for the big
> "UI correction".
>
> Do people like that plan?
>
> (soon after 1.5.2)
>
>  - start accepting "<something>:./<path>" as "relative to where I am".
>
>  - start accepting "<something>:?<string>" as "look back to find
>    the string".
>
>  - clearly explain the plan and prepare the users.
>
> (a big release in the future, perhaps 1.6.0)
>
>  - "<something>:<path>" becomes relative to where you are.
>  - stop "<something>:/<string>" and start "<something>:/<fullpath>".

I will resubmit the patch following Junio's plan. It will include the "1.6.0" features but disabled.

I would just request that we expedite, if possible, at least the later switch from "<something>:/<string>" to "<something>:/<fullpath>", since with the patch _and_ the latter behavior enabled, scripts can be updated to always insert "./" or "/" and their intent will always be clear.

Thanks,
-- 
Dana L. How  danahow@gmail.com  +1 650 804 5991 cell
Johannes Schindelin· May 4, 2007, 19:06 UTC · re: Junio C Hamano · lore

Re: [PATCH] Support ent:relative_path

Hi,
On Fri, 4 May 2007, Junio C Hamano wrote:
> (a big release in the future, perhaps 1.6.0)
> 
>  - "<something>:<path>" becomes relative to where you are.

FWIW I still find that unintuitive. I know "<something>:<path>" from ssh, and there it does not change meaning depending on where I am. IMHO in most cases you want to use git-diff anyway, which _does_ honour the current relative path.

Ciao, Dscho

Junio C Hamano· May 4, 2007, 19:22 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Support ent:relative_path

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> FWIW I still find that unintuitive. I know "<something>:<path>" from ssh, 
> and there it does not change meaning depending on where I am. IMHO in most 
> cases you want to use git-diff anyway, which _does_ honour the current 
> relative path.
There, its meaning is relative to where you are, namely "$HOME".
Johannes Schindelin· May 4, 2007, 19:31 UTC · re: Junio C Hamano · lore

Re: [PATCH] Support ent:relative_path

Hi,
On Fri, 4 May 2007, Junio C Hamano wrote:
Show 8 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > FWIW I still find that unintuitive. I know "<something>:<path>" from ssh, 
> > and there it does not change meaning depending on where I am. IMHO in most 
> > cases you want to use git-diff anyway, which _does_ honour the current 
> > relative path.
> 
> There, its meaning is relative to where you are, namely "$HOME".

No, it is relative to where I am _at the other end_. If I "cd /tmp", it still is relative to $HOME.

Now, what you want to do is changing the meaning of v1.5.1:Makefile, depending if you "cd Documentation/"ed or not.

For me, "v1.5.1:" means something similar to ssh: it is a distant revision. It is not a complete filesystem. I think of revisions as something more general than a directory, but less general than a filesystem. And thus, it makes perfect sense to me that "v1.5.1:Makefile" means the main Makefile, no matter where I am in the current repository.

Now, I agree that often you want to compare some file in the current directory to the corresponding file in a certain revision. That is why git-diff has a different idea, and indeed, a different notation, too.

Ciao, Dscho

Junio C Hamano· May 4, 2007, 19:38 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Support ent:relative_path

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 5 quoted lines
> For me, "v1.5.1:" means something similar to ssh: it is a distant 
> revision. It is not a complete filesystem. I think of revisions as 
> something more general than a directory, but less general than a 
> filesystem. And thus, it makes perfect sense to me that "v1.5.1:Makefile" 
> means the main Makefile, no matter where I am in the current repository.
I see merits in both sides' arguments.  

Saying "path out of THIS version" anchors your mindset at the top of that version, so in that sense v1.5.1:Makefile should mean the toplevel no matter where you are. However...

> Now, I agree that often you want to compare some file in the current 
> directory to the corresponding file in a certain revision. That is why 
> git-diff has a different idea, and indeed, a different notation, too.

... don't you expect "git show HEAD@{yesterday}:git.txt" while in Documentation/ subdirectory to work? I wonder if the current alternative "git show HEAD@{yesterday}:Documentation/git.txt" a bit unintuitive.

Johannes Schindelin· May 4, 2007, 19:42 UTC · re: Junio C Hamano · lore

Re: [PATCH] Support ent:relative_path

Hi,
On Fri, 4 May 2007, Junio C Hamano wrote:
> ... don't you expect "git show HEAD@{yesterday}:git.txt" while
> in Documentation/ subdirectory to work?

Actually, no. I expect "git diff HEAD@{yesterday} git.txt" to work fine, and "git show HEAD@{yesterday}:git.c" also, both when in Documentation/, because I expect "git show HEAD@{yesterday}:git.c" to work _also_ when I set GIT_DIR=/path/to/my/bare-repo.git/ before that, while I do not expect the call to git-diff to work (for obvious reasons).

Ciao, Dscho

Alex Riesen· May 4, 2007, 20:21 UTC · re: Junio C Hamano · lore

Re: [PATCH] Support ent:relative_path

Junio C Hamano, Fri, May 04, 2007 19:17:39 +0200:
Show 8 quoted lines
> Do people like that plan?
> 
> (soon after 1.5.2)
> 
>  - start accepting "<something>:./<path>" as "relative to where I am".
> 
>  - start accepting "<something>:?<string>" as "look back to find
>    the string".

Still don't like the feature. It makes things special: suddenly we have to be careful about not having files with a "?" as their first symbol (well, scripts always can prepend "./" or "/")

And it is hard to use: only prefix, must be unambiguous and is case sensitive.

Show 6 quoted lines
>  - clearly explain the plan and prepare the users.
> 
> (a big release in the future, perhaps 1.6.0)
> 
>  - "<something>:<path>" becomes relative to where you are.
>  - stop "<something>:/<string>" and start "<something>:/<fullpath>".
I like the plan.
Jakub Narebski· May 4, 2007, 23:36 UTC · re: Junio C Hamano · lore

Re: [PATCH] Support ent:relative_path

Junio C Hamano wrote:
Show 11 quoted lines
> (soon after 1.5.2)
> 
>  - start accepting "<something>:./<path>" as "relative to where I am".
>  - start accepting "<something>:?<string>" as "look back to find
>    the string".
>  - clearly explain the plan and prepare the users.
> 
> (a big release in the future, perhaps 1.6.0)
> 
>  - "<something>:<path>" becomes relative to where you are.
>  - stop "<something>:/<string>" and start "<something>:/<fullpath>".

I'm not sure about "<tree-ish>:<path>" with <path> being relative by default. For me it is <path> in <tree-ish> (like in "git-ls-tree -r <tree-ish>" result).

The "<tree-ish>:./<path>" is a good syntax I think.
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
Junio C Hamano· May 5, 2007, 00:04 UTC · re: Jakub Narebski · lore

Re: [PATCH] Support ent:relative_path

Jakub Narebski <jnareb@gmail.com> writes:
> I'm not sure about "<tree-ish>:<path>" with <path> being relative by
> default. For me it is <path> in <tree-ish> (like in 
> "git-ls-tree -r <tree-ish>" result).
That's right (and Dscho is also).

"v1.5.1:git.c" IS "git.c that appears at the toplevel of v1.5.1's tree."

Ok, for now let's forget about this relative stuff.
Dana How· May 5, 2007, 00:52 UTC · re: Junio C Hamano · lore

Re: [PATCH] Support ent:relative_path

On 5/4/07, Junio C Hamano <junkio@cox.net> wrote:
Show 11 quoted lines
> Jakub Narebski <jnareb@gmail.com> writes:
> > I'm not sure about "<tree-ish>:<path>" with <path> being relative by
> > default. For me it is <path> in <tree-ish> (like in
> > "git-ls-tree -r <tree-ish>" result).
>
> That's right (and Dscho is also).
>
> "v1.5.1:git.c" IS "git.c that appears at the toplevel of
> v1.5.1's tree."
>
> Ok, for now let's forget about this relative stuff.

Hmm, most of the work I do in the parts of our perforce repository I want to convert to git is far enough down that the paths have 6 in-repo path components. I don't want to type all those when I want to fetch an older version with git-show. Everything I do is relative. In fact, I think perforce supports typing absolute paths, (using an 8-character prefix!) but I have never used it, nor would I if it were shorter.

-- 
Dana L. How  danahow@gmail.com  +1 650 804 5991 cell
Jakub Narebski· May 5, 2007, 01:06 UTC · re: Dana How · lore

Re: [PATCH] Support ent:relative_path

Dana How wrote:
> On 5/4/07, Junio C Hamano <junkio@cox.net> wrote:
>> Jakub Narebski <jnareb@gmail.com> writes:
Show 19 quoted lines
>>> I'm not sure about "<tree-ish>:<path>" with <path> being relative by
>>> default. For me it is <path> in <tree-ish> (like in
>>> "git-ls-tree -r <tree-ish>" result).
>>
>> That's right (and Dscho is also).
>>
>> "v1.5.1:git.c" IS "git.c that appears at the toplevel of
>> v1.5.1's tree."
>>
>> Ok, for now let's forget about this relative stuff.
> 
> Hmm, most of the work I do in the parts of our
> perforce repository I want to convert to git is
> far enough down that the paths have 6 in-repo path components.
> I don't want to type all those when I want to fetch an
> older version with git-show.  Everything I do is relative.
> In fact,  I think perforce supports typing absolute paths,
> (using an 8-character prefix!) but I have never used it,
> nor would I if it were shorter.

I think the consensus is to use <tree-ish>:./<relative-path> for relative paths, and <tree-ish>:<path> for absolute (well, in the meaning that it counts from top of _tree-ish_, and tree-ish needs not to be top tree / commit tree).

And I think 6 in-repo path components means not very well thought directory hierarchy, I think...

-- 
Jakub Narebski
Poland
Junio C Hamano· May 5, 2007, 01:15 UTC · re: Dana How · lore

Re: [PATCH] Support ent:relative_path

"Dana How" <danahow@gmail.com> writes:
Show 16 quoted lines
> On 5/4/07, Junio C Hamano <junkio@cox.net> wrote:
>> Jakub Narebski <jnareb@gmail.com> writes:
>> > I'm not sure about "<tree-ish>:<path>" with <path> being relative by
>> > default. For me it is <path> in <tree-ish> (like in
>> > "git-ls-tree -r <tree-ish>" result).
>>
>> That's right (and Dscho is also).
>>
>> "v1.5.1:git.c" IS "git.c that appears at the toplevel of
>> v1.5.1's tree."
>>
>> Ok, for now let's forget about this relative stuff.
>
> Hmm, most of the work I do in the parts of our
> perforce repository I want to convert to git is
> far enough down...

The key word in my statement is "for now". I simply declared that I do not want to think about it further until 1.5.2 final, that's all.

I am not opposed to give an escape hatch for your problem; I am quite in favor of that, and I think Alex/J6t's "./" syntax is a reasonable compromise.

Alex Riesen· May 4, 2007, 08:20 UTC · re: Dana How · lore

Re: [PATCH] Support ent:relative_path

On 5/4/07, Dana How <danahow@gmail.com> wrote:
> +       if (!strcmp(var, "core.relativepaths")) {
> +               assume_relative_paths = git_config_bool(var, value);
> +               return 0;
> +       }
Maybe we should use relative path always, unconditionally.

← back to recent threads