login
Search
If we were to support relative paths, I think it would be useful and consistent if a relative path found in ".git/config" is relative to the work tree root, in "config" in a bare repository relative to the bare repository, and in "$HOME/.gitconfig" relative to $HOME. I am not sure what a relative path in "/etc/gitconfig" should be relative to, though. However, this has a technical difficulty. When configuration values are read, the code that knows what the value means does not in general know It is quite likely that somebody would want you to interpret "~name/" if you advertize that you support "~/", so you would need to call getpwuid() eventually if you go down this path. I wonder how this would affect Windows folks. What are the paths valued configuration variables other than excludesfile that we would want to support? There was a topic to allow mail-aliases lookup for parameters given to the "--author" option today, and send-email takes aliasfile configuration. Because the latter is a script, we would need a "--path" option to "git config" (the idea is similar to existing "--bool" option) so that calling scripts can ask the same "magic" performed to configuration variables' values before being reported. --
>>>>> On 2008-08-22 14:10 PDT, Junio C Hamano writes:
Junio> Karl Chen <quarl@cs.berkeley.edu> writes:
>> Another idea is to have a non-absolute path be interpreted
>> relative to the location of .gitconfig.
Junio> If we were to support relative paths, I think it would
Junio> be useful and consistent if a relative path found in
Junio> ".git/config" is relative to the work tree root, in
Junio> "config" in a bare repository relative to the bare
Junio> repository, and in "$HOME/.gitconfig" relative to
Junio> $HOME.
Makes sense to support it everywhere. For .git/config, isn't it
more consistent for it to be relative to .git?
Junio> I am not sure what a relative path in "/etc/gitconfig"
Junio> should be relative to, though.
Why not just relative to the location of that file? Normally
/etc, but if some distro customizes the location of /etc/gitconfig
(/etc/git/config), or on non-Linux/posix systems it's somewhere
else, or git is installed in /usr/local or /opt or $HOME, then
it's still relative to the location of system gitconfig.
Junio> However, this has a technical difficulty. When
Junio> configuration values are read, the code that knows what
Junio> the value means does not in general know which
Junio> configuration file is being read from.
Sounds like a refactoring issue.
Junio> It is quite likely that somebody would want you to
Junio> interpret "~name/" if you advertize that you support
Junio> "~/", so you would need to call getpwuid() eventually
Junio> if you go down this path. I wonder how this would
Junio> affect Windows folks.
I would be happy either way. Though since git uses getenv("HOME")
to find ~/.gitconfig, I can see arguments for looking for the
ignore file there also, in case it's different.
Junio> we would need a "--path" option to "git config" (the
Junio> idea is similar to existing "--bool" option) so that
Junio> calling scripts can ask the same ...Consistency and usefulness are different things. Suppose you want as the
upstream of your project maintain and distribute a mail-alias list in-tree
(say, the file is at the root level, CONTRIBUTORS), and you suggest
contributors to use it when using "commit --author".
Which one do you want to write in your README:
[user]
nicknamelistfile = ../CONTRIBUTORS
or
[user]
nicknamelistfile = CONTRIBUTORS
I do not have fundamental objection to what you are trying to achieve
(i.e. being able to say "relative to $HOME"). I personally think the
approach you took in your patch (i.e. only support "~/" and use $HOME,
without any other fancy stuff) is a sensible first cut for that issue.
I just pointed out possible design issues about the future direction after
that first cut. When I make comments on design-level issues, I rarely
read the patch itself very carefully, so it is a different issue if your
particular implementation in the patch is the best implementation of that
first cut approach.
--
Couldn't the exact opposite argument be made for "suppose you want to put the mail-alias file in a repo-specific directory that was not tracked?" I.e., you are trading off "CONTRIBUTORS" against ".git/CONTRIBUTORS". So which one inconveniences the smallest number of people is really a question of what people want to do with such pointers (and since we don't support any yet, we don't really know...). But more worrisome to me is that the working directory and git directory do not necessarily follow a "../" and ".git/" relationship. How would you resolve "../foo" with: GIT_DIR=/path/to/other/place git ... or git --work-tree=/path/to/other/place ? If you want to be able to point to either, I suspect we are better off simply introducing some basic substitutions like $GIT_DIR and $GIT_WORK_TREE. Maybe even just allow environment variable expansion, and then promise to set those variables, which takes care of $HOME automagically. -Peff --
No, I couldn't ;-) Why would you write what you wrote in README? Anything you store in .git is not propagated, so the instruction would not likely to be "store it in .git/CONTRIBUTORS and point at it". There is no merit in forcing users to standardize on "in .git". The instruction would be to "store it anywhere you want, and point at it". The example I gave is very different. It points at an in-tree thing, and anybody who has worktree checked out will have it _at the location I as the README writer expect it to be_. That is the difference that makes the exact opposite argument much weaker. That's why I suggested "relative to work tree if in .git/config, or gitdir if in config of a bare repository", although honestly speaking I do not Because we haven't deprecated core.worktree (or $GIT_WORK_TREE) yet, your suggestion has an obvious chicken-and-egg problem, even though otherwise I think it makes perfect sense and very much like it. Perhaps we should rid of the worktree that is separate and floats unrelated to where $GIT_DIR is. --
Ah, right. I still think there is a little bit of convenience when you are doing something totally personal (i.e., not putting it in a README, but rather just wanting to store the referenced file in .git for the sake of simplicity). But in that case, it is only slightly less convenient to just point to the full path. So your example trumps this, since you have You might be able to get around that by lazily filling in the variable. IOW, expand it at the point-of-use rather than while reading the config. However, the point of use might easily have something to do with reading config, so that re-creates the cycle. In general, I think we treat config as order-independent. We could make the use of such variables order-dependent (i.e., if you haven't set core.worktree, then we give you the value without having set it, and we recalculate later. Confusing results, but at least a simple rule to understand). I think there actually are a few other order-dependent things in the config, like the order of multi-value keys like push and fetch refspecs. Would you want this expansion only for specially marked variables, or for all variables? I like the concept of general templates for config I assumed people were actually using it, which is why it was implemented. -Peff --
Judging from the occasional "I tried core.worktree but it does not work in this and that situations" I see here and on #git, my impression is that new people try it, saying "git is cool -- unlike cvs that sprinkles those ugly CVS directories all over the place, it only contaminates my work tree with a single directory '.git' and nothing else. Ah, wait --- what's this core.worktree thing? Can I get rid of that last one as well? That sounds even cooler". IOW, I do not think it is really _needed_ per-se as a feature, but it was done because it was thought to be doable, which unfortunately turned out to involve hair-pulling complexity that the two attempts that led to the current code still haven't resolved. I really wish we do not have to worry about that anymore. --
Well, as a non-user of this feature, I certainly have no argument against taking it out. Maybe the subject line will pull some other people into the discussion. -Peff --
Heh, if we are to do the attention-getter, let's do so more strongly ;-) --
Does this include removing of --work-tree as well? The git backend of Pootle (http://translate.sourceforge.net/wiki/) uses it. Also, here is a question: $ git --git-dir git/.git --work-tree git diff --stat|tail -n 1 1443 files changed, 0 insertions(+), 299668 deletions(-) So, it's like it thinks every file is removed. But then: $ cd git $ git diff --stat|wc -l 0 is this a bug, or a user error? Thanks.
Interesting. Does it use it because it can (meaning, --work-tree is supposed to work), or because --work-tree is the cleanest way to do what it wants to do (if the feature worked properly, that is, which is not the I think it is among the many other things that falls into "the two attempts still haven't resolved" category. --
It's like: The current working directory is like /usr/lib/python2.5/site-packages/Pootle. The git repository is under /some/other/path/outside/usr. Then Pootle has two possibilities: 1) save the current directory, change to /some/other, execute git, and change the directory back 2) use git --work-tree / --git-dir I guess the second form is more elegant. Of course if it is decided that this option will be removed then the old form can be still used, but I I'm unfamiliar with this part of the codebase, so in case somebody other could look at it, that would be great, but I'm happy with write a testcase for it. (Or in case nobody cares, I can try to fix it, but that may take a bit more time.)
Because "git diff" did not call setup_work_tree(). The same happens for "git diff-index" that someone reported recently. IIRC "git diff-files" has the same problem. -- Duy --
This makes it possible to use git diff when we are outside the repo but --work-tree and --git-dir is used. Signed-off-by: Miklos Vajna <vmiklos@frugalware.org> --- Thanks, that was the problem. builtin-diff-files.c | 1 + builtin-diff-index.c | 1 + builtin-diff.c | 1 + 3 files changed, 3 insertions(+), 0 deletions(-) diff --git a/builtin-diff-files.c b/builtin-diff-files.c index 9bf10bb..4802e00 100644 --- a/builtin-diff-files.c +++ b/builtin-diff-files.c @@ -19,6 +19,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix) int result; unsigned options = 0; + setup_work_tree(); init_revisions(&rev, prefix); git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ rev.abbrev = 0; diff --git a/builtin-diff-index.c b/builtin-diff-index.c index 17d851b..b8e0656 100644 --- a/builtin-diff-index.c +++ b/builtin-diff-index.c @@ -16,6 +16,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) int i; int result; + setup_work_tree(); init_revisions(&rev, prefix); git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ rev.abbrev = 0; diff --git a/builtin-diff.c b/builtin-diff.c index 7ffea97..86f9255 100644 --- a/builtin-diff.c +++ b/builtin-diff.c @@ -244,6 +244,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) int nongit; int result = 0; + setup_work_tree(); /* * We could get N tree-ish in the rev.pending_objects list. * Also there could be M blobs there, and P pathspecs. -- 1.6.0.rc3.17.gc14c8.dirty --
No. git-diff has too many modes, some does not need worktree. This forces worktree on all modes. -- Duy --
On Mon, Aug 25, 2008 at 09:46:37PM +0700, Nguyen Thai Ngoc Duy <pclouds@gma= Ah, yes. I just wanted to say that I forgot do a 'make test' and actually this breaks at least t0020-crlf.sh. I'll post a fixed patch in a bit. Sorry.
This makes it possible to use git diff when we are outside the repo but --work-tree and --git-dir is used. Signed-off-by: Miklos Vajna <vmiklos@frugalware.org> --- builtin-diff-files.c | 1 + builtin-diff-index.c | 2 ++ builtin-diff.c | 1 + 3 files changed, 4 insertions(+), 0 deletions(-) diff --git a/builtin-diff-files.c b/builtin-diff-files.c index 9bf10bb..4802e00 100644 --- a/builtin-diff-files.c +++ b/builtin-diff-files.c @@ -19,6 +19,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix) int result; unsigned options = 0; + setup_work_tree(); init_revisions(&rev, prefix); git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ rev.abbrev = 0; diff --git a/builtin-diff-index.c b/builtin-diff-index.c index 17d851b..5510291 100644 --- a/builtin-diff-index.c +++ b/builtin-diff-index.c @@ -29,6 +29,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) else usage(diff_cache_usage); } + if (!cached) + setup_work_tree(); if (!rev.diffopt.output_format) rev.diffopt.output_format = DIFF_FORMAT_RAW; diff --git a/builtin-diff.c b/builtin-diff.c index 7ffea97..57da6ed 100644 --- a/builtin-diff.c +++ b/builtin-diff.c @@ -279,6 +279,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) diff_no_index(&rev, argc, argv, nongit, prefix); /* Otherwise, we are doing the usual "git" diff */ + setup_work_tree(); rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index; if (nongit) -- 1.6.0.rc3.17.gc14c8.dirty --
At least builtin_diff_blobs() and builtin_diff_tree() won't need worktree, so NACK again. Anyway I'm not familiar with diff*. Junio should know these better. -- Duy --
How about doing it this way then?
* diff-files is about comparing with work tree, so it obviously needs a
work tree;
* diff-index also does;
* no-index is about random files outside git context, so it obviously
doesn't need any work tree;
* comparing two (or more) trees doesn't;
* comparing two blobs doesn't;
* comparing a blob with a random file doesn't;
What could be problematic is "git diff --cached". Strictly speaking, it
compares the index and a tree so it shouldn't need any work tree. The
same obviously applies to "git diff-index --cached".
While it is theoretically possible to have an index in a bare repository
and build your history using it without using any worktree, I do not think
it is a use case worth worrying about. As long as setup_work_tree() does
not complain and die in such a setup, "diff --cached" itself won't look at
the work tree (whereever random place setup_work_tree() sets it) at all,
so probably it is a non issue. I dunno.
I do not have a test environment that uses a separate worktree settings,
so this is obviously untested.
Perhaps people who are interested in keeping core.worktree alive can add
test scripts in t/ somewhere to help salvaging the feature?
---
builtin-diff.c | 3 +++
git.c | 4 ++--
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git i/builtin-diff.c w/builtin-diff.c
index 7ffea97..06c85da 100644
--- i/builtin-diff.c
+++ w/builtin-diff.c
@@ -114,6 +114,8 @@ static int builtin_diff_index(struct rev_info *revs,
int argc, const char **argv)
{
int cached = 0;
+
+ setup_work_tree();
while (1 < argc) {
const char *arg = argv[1];
if (!strcmp(arg, "--cached"))
@@ -207,6 +209,7 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv
int result;
unsigned int options = 0;
+ setup_work_tree();
while (1 < argc && argv[1][0] == '-') {
if (!strcmp(argv[1], "--base"))
revs->max_count = 1;
diff --git i/git.c ...This fixes "git diff", "git diff-files" and "git diff-index" to work
correctly under worktree setup. Because diff* family works in many modes
and not all of them require worktree, Junio made a nice summary
(with a little modification from me):
* diff-files is about comparing with work tree, so it obviously needs a
work tree;
* diff-index also does, except "diff-index --cached" or "diff --cached TREE"
* no-index is about random files outside git context, so it obviously
doesn't need any work tree;
* comparing two (or more) trees doesn't;
* comparing two blobs doesn't;
* comparing a blob with a random file doesn't;
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin-diff-index.c | 2 +
builtin-diff.c | 3 ++
git.c | 2 +-
t/t1501-worktree.sh | 59 ++++++++++++++++++++++++++++++++++++++++++++++++-
4 files changed, 63 insertions(+), 3 deletions(-)
diff --git a/builtin-diff-index.c b/builtin-diff-index.c
index 17d851b..0483749 100644
--- a/builtin-diff-index.c
+++ b/builtin-diff-index.c
@@ -39,6 +39,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
if (rev.pending.nr != 1 ||
rev.max_count != -1 || rev.min_age != -1 || rev.max_age != -1)
usage(diff_cache_usage);
+ if (!cached)
+ setup_work_tree();
if (read_cache() < 0) {
perror("read_cache");
return -1;
diff --git a/builtin-diff.c b/builtin-diff.c
index 7ffea97..037c303 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -122,6 +122,8 @@ static int builtin_diff_index(struct rev_info *revs,
usage(builtin_diff_usage);
argv++; argc--;
}
+ if (!cached)
+ setup_work_tree();
/*
* Make sure there is one revision (i.e. pending object),
* and there is no revision filtering parameters.
@@ -225,6 +227,7 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv
(revs->diffopt.output_format & DIFF_FORMAT_PATCH))
revs->combine_merges = revs->dense_combined_merges ...The real question was about if/why that git repository _has to be_ outside of /usr/lib/*/Pootle/. Is that because --work-tree, if worked properly, would have allowed it to be, or is that because for some external reason you are not allowed to have .git under /usr/lib/*/Pootle/ directory? If the latter, that shows the real requirement to keep supporting it as a feature, and issues around it need to be fixed. Otherwise, i.e. if it does not require use of --work-tree but it uses it only because it could, that gives us less incentive to keep --work-tree as a feature. I haven't read the breakage and fix around diff in this thread yet, but I will when I get home. Thanks to both Nguyen and you for trying to salvage --work-tree support. --
I'm not 100% sure, but I think the reason is that Pootle runs as the user 'pootle' which has write access to some /var/pootle or /home/pootle I think this is the latter, though as I mentioned previously - it can be still worked around by a "cd /other/path; git <command>; cd -" (speaking in shell commands), so it is not a "must", it would be just ugly IMHO.
Sorry for being late to the discussion.
I think there are many use cases or environments which differ
substantially from those of the "typical" developer; this implies that
they differ from those of the typical git contributor, which naturally
leads to a certain bias in discussions like this one.
"Typical" developers track source code in the proper sense (somewhere in
$HOME); on local file systems; mostly on machines where they have root
access, or least can get extra accounts (for gitosis) or a port for "git
daemon" etc; they collaborate with peers for whom basically the same
assumptions apply.
Now think of a user say in academics, who tracks "source code" for
scientific papers (somewhere in $HOME) but also needs to track, e.g.,
central web pages or other "sources" where he has partial write access
but can't have ".git" in place (and shouldn't change ownership &
permissions), but needs to be aware of changes and log own changes; on
NFS; no extra accounts but in need of an authenticated protocol (papers
in progress are private, public only when published); who collaborates
with peers for whom the same assumptions apply, except most certainly
for git usage...
Yes, that's me, but also many others, I would think and hope, at least
increasingly so. That second scenario is one where I have to cope with
how things are set up centrally, making the best possible use of git.
I would imagine that many corporate environments are basically similar,
if individual employees want to use git without central support.
These remarks apply to the discussion about an authenticated protocol
(some way for secure, private pull&push for users with access to $HOME
and maybe cgi-bins), but also here:
I need to keep .git away from the work tree for several projects. Using
--git-dir etc. leads to problems with some commands, especially
git{k,-gui,-citool}. I found the most robust solution to be an alias
(shell) which guesses the work tree (from core.worktree etc.) and cd's
there before doing ...[resend: urgh, I somehow missed the git-list when replying, so it is cc'd here] Actually, I do all of those things, and I don't use the work-tree config variable or command line options at all. :) I think the general advice with things like web access is "don't just dump your git stuff into a production area; instead, build and/or install from your git work tree into your production area". Because things like merges _can_ leave your files in a broken state. But I do recognize that there are some special circumstances where that isn't possible, and you are willing to accept the tradeoff. E.g., if the checkout is extremely large and you can't afford another copy, if you have clueless collaborators who can't understand a build procedure. And even though I expect those cases to be the exception, it seems a shame for git not to support split git-dir/work-tree setups because we really are 99% there. This code has been the source of a number of problems. I think what is really needed is somebody to look carefully at the git startup sequence and figure out a sane set of rules for the order of: - looking at env variables - looking at config - figuring out GIT_DIR and GIT_WORK_TREE - chdir'ing to top level of work tree if necessary Because we obviously have some corner cases where very confusing things are happening. This is on my long term todo list, but my git time is very short at least for the next few months. I think it would be great if somebody else wanted to take the lead on this, and I would be happy to give pointers about some of the corner cases we have already seen. -Peff --
The config variable core.excludesfile is parsed to substitute ~ and ~user with
getpw entries.
Signed-off-by: Karl Chen <quarl@quarl.org>
---
config.c | 41 +++++++++++++++++++++++++++++++++++++++--
1 files changed, 39 insertions(+), 2 deletions(-)
Based on the discussion it sounds like there are complications to
supporting relative paths (due to worktree config), and "$HOME"
(when generalized, due to bootstrapping issues with $GIT_*).
Since ~ and ~user are orthogonal to these, can I suggest going
forward with this, without blocking on those two?
I have reworked the patch to use getpw to support ~user. $HOME
can eventually be supported via $ENVVARs.
diff --git a/config.c b/config.c
index 53f04a0..6a83c64 100644
--- a/config.c
+++ b/config.c
@@ -334,6 +334,42 @@ int git_config_string(const char **dest, const char *var, const char *value)
return 0;
}
+/*
+ * Expand ~ and ~user. Returns a newly malloced string. (If input does not
+ * start with "~", equivalent to xstrdup.)
+ */
+static char *expand_userdir(const char *value) {
+ if (value[0] == '~') {
+ struct passwd *pw;
+ char *expanded_dir;
+ const char *slash = strchr(value+1, '/');
+ const char *after_username = slash ? slash : value+strlen(value);
+ if (after_username == value+1) {
+ pw = getpwuid(getuid());
+ if (!pw) die("You don't exist!");
+ } else {
+ char save = *after_username;
+ *(char*)after_username = '\0';
+ pw = getpwnam(value+1);
+ if (!pw) die("No such user: '%s'", value+1);
+ *(char*)after_username = save;
+ }
+ expanded_dir = xmalloc(strlen(pw->pw_dir) + strlen(after_username) + 1);
+ strcpy(expanded_dir, pw->pw_dir);
+ strcat(expanded_dir, after_username);
+ return expanded_dir;
+ } else {
+ return xstrdup(value);
+ }
+}
+
+int git_config_userdir(const char **dest, const char *var, const char *value) {
+ if (!value)
+ return config_error_nonbool(var);
+ *dest = expand_userdir(value);
+ return 0;
+}
+
static int ...There is user_path() in path.c that does the same thing. Watch your style: The opening brace of functions is on the next line. -- Hannes --
I think that is fine for now. One other simple possibility would be to expand _just_ $HOME, and then if we later decided to do all environment variables it would naturally encompass that. However, we might want to support "~" then anyway, so I think doing "~" first is fine. However, there are two problems with the patch: 1. It should probably re-use path.c:user_path, as Johannes mentioned. 2. There is no documentation update. Also, are there any other config variables which would benefit from this substitution (I can't think of any off-hand, but there are quite a few I don't use). -Peff --
>>>>> On 2008-08-26 17:25 PDT, Jeff King writes:
Jeff> 1. It should probably re-use path.c:user_path, as
Jeff> Johannes mentioned.
That function has the wrong interface for this task (requires
extra strdup, imposes PATH_MAX, conflates all error conditions
into returning NULL, also returns NULL if input doesn't have "~").
Do you still think it should re-use that function?
--
On the other hand you have a pair of ugly "casting a const pointer down, temporarily modifying what is const" in your version. In any case, I think their point is that these two functions are meant to do the same thing, and a unified interface would be desirable. You do not necessarily have to use user_path() from path.c, but user_path(), if its interface is coarser, could become a thin wrapper around yours, don't you think? Even better, perhaps the current callers of user_path() may benefit from finer distinction among error conditions (I didn't look, though). --
These config variables are parsed to substitute ~ and ~user with getpw
entries.
user_path() refactored into new function expand_user_path(), to allow
dynamically allocating the return buffer.
Signed-off-by: Karl Chen <quarl@quarl.org>
>>
Jeff> 1. It should probably re-use path.c:user_path, as
Jeff> Johannes mentioned.
>>
>> That function has the wrong interface for this task
>> (requires extra strdup, imposes PATH_MAX, conflates all
>> error conditions into returning NULL, also returns NULL if
>> input doesn't have "~"). Do you still think it should
>> re-use that function?
Junio> On the other hand you have a pair of ugly "casting a
Junio> const pointer down, temporarily modifying what is
Junio> const" in your version.
user_path() does the same thing; it's just obscured by the lack of
type qualifiers. I rationalized it because git has much bigger
problems if threading support were added. But I agree it's ugly.
The new patch avoids it.
Junio> In any case, I think their point is that these two
Junio> functions are meant to do the same thing, and a unified
Junio> interface would be desirable.
There is a lot of refactoring I would do were I maintaining the
code. I figured a minimally invasive change would be more easily
accepted. Anyway here is a patch to unify "~" expansion.
builtin-commit.c | 2 +-
cache.h | 2 +
config.c | 13 +++++++-
path.c | 88 +++++++++++++++++++++++++++++++++--------------------
4 files changed, 69 insertions(+), 36 deletions(-)
diff --git a/builtin-commit.c b/builtin-commit.c
index 649c8be..e510207 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -891,7 +891,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
static int git_commit_config(const char *k, const char *v, void *cb)
{
if (!strcmp(k, "commit.template"))
- return git_config_string(&template_file, k, v);
+ return ...I am not sure about !**dest here. This precludes somebody from using "".
While it might not matter here, if there are other users of
git_config_userdir(), they might want to allow a blank entry.
Also, style: there should be a newline after conditional but before
executed code. IOW, replace
if (cond) code;
with
if (cond)
1. There seem to be extra tabs in the second line, pushing the
end_username argument way too far to the right.
2. I'm not sure "strspan" is a good name for this helper, since it calls
to mind the strspn C function, which is not really related to this at
all.
3. Usually helper functions that take a non-terminated string like this
in git use the combination of (char *begin, int len) instead of two
pointers. While you are currently the only user of the helper, I
Style: omit braces on one-liner conditionals:
if (begin_username == end_username)
return getwpuid(getuid());
Also, you do a lot of early returns in your code. I think this is good,
because it makes it more readable. But that means you don't have to
worry about "else"ing the other half of the conditional, because you
See, here you end up converting back from two pointers to a pointer and
I don't think we use alloca() anywhere else. I don't know if there are
Style: more braces which can be omitted.
This function seems a little superfluous, since its semantics are so
specific to this usage. I am all for splitting into little functions,
but I think it would be quite confusing for somebody to try reusing
this. Perhaps it at least needs a comment explaining the semantics of
This really seems like premature optimization to me. The only advantage
this has over
p = strchr(s);
if (!p)
p = s + strlen(s);
is that we avoid traversing the string once. But balance that against an
assembler-optimized strchr provided by your libc. And then wonder if it
More early returns which can be removed from conditionals.
Also, some of this code ...You really should use the strbuf API here. Look for strbuf_detach() in the existing code. -- Hannes --
>>>>> On 2008-08-26 17:25 PDT, Jeff King writes:
Jeff> 2. There is no documentation update.
Relative paths and $ENVVARS would need explanation; not sure what
needs to be said about ~user since the new behavior is what people
expect to just work. Would it go in git-config.txt if something
were added?
--
Traditionally, we never has interpret ~/ or ~user/. Users expected that
they need to spell it like:
[core]
excludesfile = "/home/joe/.my-ignore-pattern"
Especially since this is called "variables", it is natural, without
explanation, to expect this not to work, just like this use of a variable
won't work:
$ EDITOR='~/bin/editor' ;# notice the single quote
$ export EDITOR;
$ git commit -a
fatal: exec ~/bin/editor failed.
Your patch improves the situation and now allow:
[core]
excludesfile = "~/.my-ignore-pattern"
If you do not document that it is now possible for this particular
configuration variable, the users will never know.
--
