"git read-tree -m" and the like require worktree
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/10 "git read-tree -m" and the like require worktree
--- builtin-read-tree.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
builtin-read-tree.c +3 -0
@@ -220,6 +220,9 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)if ((opts.dir && !opts.update))die ("--exclude-per-directory is meaningless unless -u" ); +if (opts.merge && !opts.index_only) +setup_work_tree (); +if (opts.prefix) {int pfxlen =strlen (opts.prefix);int pos;
Signature · 1 line
1.5.4.2.281.g28d0e
Hi,
How about
git read-tree -m without -i requires work tree
Hmm?
Other than that, it looks like an obvious bugfix which is independent of the rest.
Ciao,
Dscho
Quoted 7 lines · Johannes Schindelin
Hi,
How about
git read-tree -m without -i requires work tree
Hmm?
I thought that was the patch was about. Did I write the patch so bad? :(
Quoted 3 lines
Other than that, it looks like an obvious bugfix which is independent of the rest.
Agreed completely. As said in the other mail, this patch can be cherry picked well without the rest of the series (should I resend it?).
Quoted 5 lines
Ciao,
Dscho
Signature · 1 line
Duy
Hi,
Quoted 10 lines · Nguyen Thai Ngoc Duy
Quoted 5 lines · Johannes Schindelin
How about
git read-tree -m without -i requires work tree
Hmm?
I thought that was the patch was about. Did I write the patch so bad? :(
I meant as a replacement for your commit subject (which I found pretty hard to understand, harder than the patch itself).
Ciao,
Dscho
Quoted 11 lines · Johannes Schindelin
Hi,
Quoted 6 lines · Nguyen Thai Ngoc Duy
Quoted 2 lines · Johannes Schindelin
How about
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
git read-tree -m without -i requires work tree
Hmm?
I thought that was the patch was about. Did I write the patch so bad? :(
I meant as a replacement for your commit subject (which I found pretty hard to understand, harder than the patch itself).
Yes it is absolutely a better subject. Thanks.
Signature · 1 line
Duy
PATCH 2/10 Make sure setup_git_directory is called before accessing repository
In reply to a message not in the archiveRight now setup_git_env is called automatically when needed and will not die out if $GIT_DIR is invalid. The next patch no longer guarantee that, so make sure all commands have environment properly setup before using.
--- fast-import.c | 1 + hash-object.c | 10 +++++----- index-pack.c | 2 ++ 3 files changed, 8 insertions(+), 5 deletions(-)
fast-import.c +1 -0
@@ -2377,6 +2377,7 @@ int main(int argc, const char **argv) {unsigned int i, show_stats =1 ; +setup_git_directory ();git_config (git_pack_config);if (!pack_compression_seen && core_compression_seen) pack_compression_level = core_compression_level;
hash-object.c +5 -5
@@ -42,7 +42,10 @@ int main(int argc, char **argv)int prefix_length =-1 ;int no_more_flags =0 ;int hashstdin =0 ; +int nongit =0 ; + prefix =setup_git_directory_gently (&nongit); + prefix_length = prefix ?strlen (prefix) :0 ;git_config (git_default_config);for (i =1 ; i < argc; i++) {
@@ -53,11 +56,8 @@ int main(int argc, char **argv) type = argv[i]; }else if (!strcmp (argv[i],"-w" )) { -if (prefix_length <0 ) { - prefix =setup_git_directory (); - prefix_length = - prefix ?strlen (prefix) :0 ; - } +if (nongit) +die ("Not a git repository" ); write_object =1 ; }else if (!strcmp (argv[i],"--" )) {
index-pack.c +2 -0
@@ -785,7 +785,9 @@ int main(int argc, char **argv)char *index_name_buf =NULL , *keep_name_buf =NULL ;struct pack_idx_entry **idx_objects;unsigned char sha1[20 ]; +int nongit =0 ; +setup_git_directory_gently (&nongit);git_config (git_index_pack_config);for (i =1 ; i < argc; i++) {
Signature · 1 line
1.5.4.2.281.g28d0e
PATCH 3/10 Make get_git_dir() and 'git rev-parse --git-dir' absolute path
In reply to a message not in the archive--- builtin-rev-parse.c | 14 +------------- environment.c | 1 + t/t1300-repo-config.sh | 2 +- t/t1400-update-ref.sh | 4 ++-- t/t9300-fast-import.sh | 2 +- 5 files changed, 6 insertions(+), 17 deletions(-)
builtin-rev-parse.c +1 -13
@@ -496,19 +496,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)continue ; }if (!strcmp (arg,"--git-dir" )) { -const char *gitdir =getenv (GIT_DIR_ENVIRONMENT ); -static char cwd[PATH_MAX ]; -if (gitdir) { -puts (gitdir); -continue ; - } -if (!prefix) { -puts (".git" ); -continue ; - } -if (!getcwd (cwd,PATH_MAX )) -die ("unable to get current working directory" ); -printf ("%s/.git\n" , cwd); +puts (get_git_dir ());continue ; }if (!strcmp (arg,"--is-inside-git-dir" )) {
environment.c +1 -0
@@ -51,6 +51,7 @@ static void setup_git_env(void) git_dir =getenv (GIT_DIR_ENVIRONMENT );if (!git_dir) git_dir =DEFAULT_GIT_DIR_ENVIRONMENT ; + git_dir =xstrdup (make_absolute_path (git_dir)); git_object_dir =getenv (DB_ENVIRONMENT );if (!git_object_dir) { git_object_dir =xmalloc (strlen (git_dir) +9 );
t/t1300-repo-config.sh +1 -1
@@ -497,7 +497,7 @@ test_expect_success numbers ' ' cat > expect <<EOF -fatal: bad config value for 'aninvalid.unit' in .git/config +fatal: bad config value for 'aninvalid.unit' in $(pwd)/.git/config EOF test_expect_success 'invalid unit' '
t/t1400-update-ref.sh +2 -2
@@ -169,7 +169,7 @@ test_expect_success \ 'rm -f o e git rev-parse --verify "master@{2005-05-26 23:33:01}" >o 2>e && test '"$B"' = $(cat o) && - test "warning: Log .git/logs/'"$m has gap after $gd"'." = "$(cat e)"' + test "warning: Log '"$(pwd)"'/.git/logs/'"$m has gap after $gd"'." = "$(cat e)"' test_expect_success \ 'Query "master@{2005-05-26 23:38:00}" (middle of history)' \ 'rm -f o e
@@ -187,7 +187,7 @@ test_expect_success \ 'rm -f o e git rev-parse --verify "master@{2005-05-28}" >o 2>e && test '"$D"' = $(cat o) && - test "warning: Log .git/logs/'"$m unexpectedly ended on $ld"'." = "$(cat e)"' + test "warning: Log '"$(pwd)"'/.git/logs/'"$m unexpectedly ended on $ld"'." = "$(cat e)"' rm -f .git/$m .git/logs/$m expect
t/t9300-fast-import.sh +1 -1
@@ -510,7 +510,7 @@ test_expect_success \ 'git-fast-import --export-pack-edges=edges.list <input' cat >expect <<EOF -.git/objects/pack/pack-.pack: `git rev-parse --verify export-boundary` +$(pwd)/.git/objects/pack/pack-.pack: `git rev-parse --verify export-boundary` EOF test_expect_success \ 'I: verify edge list' \
Signature · 1 line
1.5.4.2.281.g28d0e
Re: [PATCH 03/10] Make get_git_dir() and 'git rev-parse --git-dir' absolute path
Hi,
Quoted 2 lines · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
I do not like this change. It is IMO completely unnecessary, and might break assumptions.
Ciao,
Dscho
Re: [PATCH 03/10] Make get_git_dir() and 'git rev-parse --git-dir' absolute path
Quoted 10 lines · Johannes Schindelin
Hi,
Quoted 2 lines · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>I do not like this change. It is IMO completely unnecessary, and might break assumptions.
It could. And I tried my best to make sure it did not break anything. It is also documented (if not clearly, I can correct the document). While it's not really necessary, it would be IMHO a good change as you would not have to rely on cwd anymore (that would mean whether moving cwd by prefix or not would no longer matter).
Quoted 3 lines
Ciao,
Dscho
Signature · 1 line
Duy
Re: [PATCH 03/10] Make get_git_dir() and 'git rev-parse --git-dir' absolute path
Hi,
Quoted 11 lines · Nguyen Thai Ngoc Duy
Quoted 6 lines · Johannes Schindelin
Quoted 1 line · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>I do not like this change. It is IMO completely unnecessary, and might break assumptions.
It could. And I tried my best to make sure it did not break anything.
That is contradicting. I think that whatever you tried, once you let rev-parse --git-dir return only absolute paths, you already risked breakage.
Besides, I remember that Junio specifically requested that I would _not_ do something like that (this was for --show-cdup, but I think it really applies here, too).
Quoted 3 lines
While it's not really necessary, it would be IMHO a good change as you would not have to rely on
cwdanymore (that would mean whether moving cwd by prefix or not would no longer matter).
I do not see the problem. And therefore I do not see that this patch solves anything.
Ciao,
Dscho
Re: [PATCH 03/10] Make get_git_dir() and 'git rev-parse --git-dir' absolute path
Quoted 18 lines · Johannes Schindelin
Hi,
Quoted 4 lines · Nguyen Thai Ngoc Duy
On Tue, Mar 11, 2008 at 8:20 PM, Johannes Schindelin <[email protected]> wrote:
Quoted 8 lines
Quoted 6 lines · Nguyễn Thái Ngọc Duy
Quoted 2 lines
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>I do not like this change. It is IMO completely unnecessary, and might break assumptions.
It could. And I tried my best to make sure it did not break anything.
Quoted 7 lines
That is contradicting. I think that whatever you tried, once you let rev-parse --git-dir return only absolute paths, you already risked breakage.
Besides, I remember that Junio specifically requested that I would _not_ do something like that (this was for --show-cdup, but I think it really applies here, too).
I might have lost track. Could you provide me the link for reference? Thanks.
Quoted 6 lines
Quoted 3 lines
While it's not really necessary, it would be IMHO a good change as you would not have to rely on
cwdanymore (that would mean whether moving cwd by prefix or not would no longer matter).I do not see the problem. And therefore I do not see that this patch solves anything.
It can simplify things (in other patches, such as builtin-config.c changes). Though without it things just run fine (with a little more complication in prefix handling). Recent mails about git-am running in subdirectory (Message-ID:
[email protected]) made me think there was room for this.
It is also prerequisite for later changes in the series (IIRC, removing prefix from setup*_gently() needs it).
Quoted 3 lines
Ciao,
Dscho
Signature · 1 line
Duy
Re: [PATCH 03/10] Make get_git_dir() and 'git rev-parse --git-dir' absolute path
Hi,
Quoted 8 lines · Nguyen Thai Ngoc Duy
Quoted 3 lines · Johannes Schindelin
Besides, I remember that Junio specifically requested that I would _not_ do something like that (this was for --show-cdup, but I think it really applies here, too).
I might have lost track. Could you provide me the link for reference? Thanks.
http://article.gmane.org/gmane.comp.version-control.git/53993/match=show+cdup
This is how you can find it yourself next time (and how I found it):
Go to gmane, search for "show-cdup", then select Junio as writer. It is already the second hit, which was obvious by the email's title.
Quoted 9 lines
Quoted 2 lines
I do not see the problem. And therefore I do not see that this patch solves anything.
It can simplify things (in other patches, such as
builtin-config.cchanges). Though without it things just run fine (with a little more complication in prefix handling). Recent mails about git-am running in subdirectory (Message-ID:
[email protected]) made me think there was room for this.
Okay, I see that you want to simplify things. However, I am not at all convinced that your patch series does that.
However, instead of continuing this thread, I will try to find some time later this week to look into simplifying the work-tree logic again.
If I find things, I will discuss them on the list.
Ciao,
Dscho
PATCH 4/10 Make setup_work_tree() return new prefix
In reply to a message not in the archiveworktree setup inside setup_git_directory*() is not perfect. It does not take core.worktree into account. So when you do setup_work_tree(), the real work tree may be not the one setup_git_directory*() gives you. You need a new prefix in that case.
This also effectively reverts dd5c8af (Fix setup_git_directory_gently() with relative GIT_DIR & GIT_WORK_TREE). The problem is IMHO that git-diff does not setup worktree when it needs it, so setting up worktree from setup_git_directory_gently() is a fix in a wrong place.
--- builtin-blame.c | 4 +- builtin-diff-files.c | 4 ++- builtin-diff.c | 4 ++- builtin-ls-files.c | 8 ++-- builtin-read-tree.c | 2 +- builtin-rev-parse.c | 2 + builtin-rm.c | 2 +- cache.h | 2 +- git.c | 2 +- setup.c | 84 ++++++++++++++++---------------------------------- 10 files changed, 45 insertions(+), 69 deletions(-)
builtin-blame.c +2 -2
@@ -2291,6 +2291,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)/* (3) */ if (argc <= i)usage (blame_usage); + prefix =setup_work_tree (prefix); path =add_prefix (prefix, argv[i]);if (i +1 == argc -1 ) { final_commit_name = argv[i +1 ];
@@ -2306,7 +2307,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)else if (i != argc -1 )usage (blame_usage);/* garbage at end */ -setup_work_tree ();if (!has_path_in_work_tree (path))die ("cannot stat path %s: %s" , path,strerror (errno));
@@ -2354,7 +2354,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)* do not default to HEAD, but use the working tree * or "--contents". */ -setup_work_tree (); +setup_work_tree (prefix); sb.final =fake_working_tree_commit (path, contents_from);add_pending_object (&revs, &(sb.final->object),":" ); }
builtin-diff-files.c +3 -1
@@ -26,8 +26,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)if (!setup_diff_no_index (&rev, argc, argv, nongit, prefix)) argc =0 ; -else +else { + rev.prefix =setup_work_tree (prefix); argc =setup_revisions (argc, argv, &rev,NULL ); + }if (!rev.diffopt.output_format) rev.diffopt.output_format =DIFF_FORMAT_RAW ; result =run_diff_files_cmd (&rev, argc, argv);
builtin-diff.c +3 -1
@@ -244,8 +244,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)if (!setup_diff_no_index (&rev, argc, argv, nongit, prefix)) argc =0 ; -else +else { + rev.prefix =setup_work_tree (prefix); argc =setup_revisions (argc, argv, &rev,NULL ); + }if (!rev.diffopt.output_format) { rev.diffopt.output_format =DIFF_FORMAT_PATCH ;if (diff_setup_done (&rev.diffopt) <0 )
builtin-ls-files.c +4 -4
@@ -23,7 +23,7 @@ static int show_valid_bit;static int line_terminator ='\n' ;static int prefix_len; -static int prefix_offset; +static int prefix_offset =-1 ;static const char **pathspec;static int error_unmatch;static char *ps_matched;
@@ -435,8 +435,6 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)struct dir_struct dir;memset (&dir,0 ,sizeof (dir)); -if (prefix) - prefix_offset =strlen (prefix);git_config (git_default_config);for (i =1 ; i < argc; i++) {
@@ -569,7 +567,9 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix) }if (require_work_tree && !is_inside_work_tree ()) -setup_work_tree (); + prefix =setup_work_tree (prefix); +if (prefix_offset ==-1 ) + prefix_offset = prefix ?strlen (prefix) :0 ; pathspec =get_pathspec (prefix, argv + i);
builtin-read-tree.c +1 -1
@@ -221,7 +221,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)die ("--exclude-per-directory is meaningless unless -u" );if (opts.merge && !opts.index_only) -setup_work_tree (); +setup_work_tree (NULL );if (opts.prefix) {int pfxlen =strlen (opts.prefix);
builtin-rev-parse.c +2 -0
@@ -472,6 +472,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)continue ; }if (!strcmp (arg,"--show-prefix" )) { +if (is_inside_work_tree ()) + prefix =setup_work_tree (prefix);if (prefix)puts (prefix);continue ;
builtin-rm.c +1 -1
@@ -156,7 +156,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)usage_with_options (builtin_rm_usage, builtin_rm_options);if (!index_only) -setup_work_tree (); + prefix =setup_work_tree (prefix); pathspec =get_pathspec (prefix, argv); seen =NULL ;
cache.h +1 -1
@@ -315,7 +315,7 @@ extern const char *get_git_work_tree(void);#define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"extern const char **get_pathspec (const char *prefix,const char **pathspec); -extern void setup_work_tree (void ); +extern const char *setup_work_tree (const char *prefix);extern const char *setup_git_directory_gently (int *);extern const char *setup_git_directory (void );extern const char *prefix_path (const char *prefix,int len,const char *path);
git.c +1 -1
@@ -241,7 +241,7 @@ static int run_command(struct cmd_struct *p, int argc, const char **argv)if (p->option &USE_PAGER )setup_pager ();if (p->option &NEED_WORK_TREE ) -setup_work_tree (); + prefix =setup_work_tree (prefix);trace_argv_printf (argv,"trace: built-in: git" );
setup.c +26 -55
102 lines not on this page · raw message
Re: [PATCH 04/10] Make setup_work_tree() return new prefix
Hi,
Quoted 4 lines · Nguyễn Thái Ngọc Duy
worktree setup inside setup_git_directory*() is not perfect. It does not take core.worktree into account. So when you do
setup_work_tree(), the real work tree may be not the one setup_git_directory*() gives you. You need a new prefix in that case.
Probably the real fix would be to setup_git_directory(), no?
Ciao,
Dscho
Re: [PATCH 04/10] Make setup_work_tree() return new prefix
Quoted 11 lines · Johannes Schindelin
Hi,
Quoted 4 lines · Nguyễn Thái Ngọc Duy
worktree setup inside setup_git_directory*() is not perfect. It does not take core.worktree into account. So when you do
setup_work_tree(), the real work tree may be not the one setup_git_directory*() gives you. You need a new prefix in that case.Probably the real fix would be to
setup_git_directory(), no?
setup_work_tree() does the job fine. Why duplicate the code one more? In the old days, setup_git_directory() was just a simple wrapper around setup_git_dir_gently(). I wanted to move it back to those days. And setup_git_directory() is fixed in "Completely move out worktree setup from setup_git_directory_gently()" so setup_git_directory() does contain the real fix in the end.
Quoted 2 lines
Ciao,
Dscho
Signature · 1 line
Duy
PATCH 5/10 http-push: Avoid calling setup_git_directory() twice
In reply to a message not in the archive--- http-push.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
http-push.c +2 -2
@@ -2175,7 +2175,7 @@ int main(int argc, char **argv)struct ref *ref;char *rewritten_url =NULL ; -setup_git_directory (); +const char *prefix =setup_git_directory (); remote =xcalloc (sizeof (*remote),1 );
@@ -2382,7 +2382,7 @@ int main(int argc, char **argv) commit_argv[3 ] = old_sha1_hex; commit_argc++; } -init_revisions (&revs,setup_git_directory ()); +init_revisions (&revs, prefix);setup_revisions (commit_argc, commit_argv, &revs,NULL );free (new_sha1_hex);if (old_sha1_hex) {
Signature · 1 line
1.5.4.2.281.g28d0e
Re: [PATCH 05/10] http-push: Avoid calling setup_git_directory() twice
Hi,
Quoted 2 lines · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
This fix is completely independent of the rest of your series.
Ciao,
Dscho
Re: [PATCH 05/10] http-push: Avoid calling setup_git_directory() twice
Quoted 9 lines · Johannes Schindelin
Hi,
Quoted 2 lines · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>This fix is completely independent of the rest of your series.
Not really. It would have no impact (good or bad) if sent separately. But it is required by the series (otherwise it would die() under certain condition because prefix cannot be recomputed properly).
Signature · 1 line
Duy
Re: [PATCH 05/10] http-push: Avoid calling setup_git_directory() twice
Hi,
Quoted 5 lines · Nguyen Thai Ngoc Duy
Quoted 2 lines · Johannes Schindelin
On Sun, 2 Mar 2008, Nguy�n Th�i Ng�c Duy wrote:
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Nguy�n Th�i Ng�c Duy <[email protected]>This fix is completely independent of the rest of your series.
Not really. It would have no impact (good or bad) if sent separately. But it is required by the series (otherwise it would
die()under certain condition because prefix cannot be recomputed properly).
Well, it _is_ independent. Either you say "we want to have this as an example where you can call setup_git_directory() twice" (which I would not find all that bad), or you say "this was calling setup_git_directory() twice, fix that".
In any case, it should be separate from this series.
Back to the subject of this series: as I stated already, I do not like the separation of work-tree and git directory, and I especially do not like how much you had to work outside setup.c. I think changing the semantics for all callers was not necessary, and in fact, not desirable.
Ciao,
Dscho
Re: [PATCH 05/10] http-push: Avoid calling setup_git_directory() twice
Quoted 19 lines · Johannes Schindelin
Hi,
Quoted 5 lines · Nguyen Thai Ngoc Duy
Quoted 2 lines · Johannes Schindelin
On Sun, 2 Mar 2008, Nguyn Thái Ng÷c Duy wrote:
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Nguyn Thái Ng÷c Duy <[email protected]>This fix is completely independent of the rest of your series.
Not really. It would have no impact (good or bad) if sent separately. But it is required by the series (otherwise it would
die()under certain condition because prefix cannot be recomputed properly).
Quoted 6 lines
Well, it _is_ independent. Either you say "we want to have this as an example where you can call
setup_git_directory()twice" (which I would not find all that bad), or you say "this was callingsetup_git_directory()twice, fix that".In any case, it should be separate from this series.
My point stands still. It is merely cleanup without harm or goodness if seperated from the series. This patch does not fix the real problem.
Quoted 4 lines
Back to the subject of this series: as I stated already, I do not like the separation of work-tree and git directory, and I especially do not like how much you had to work outside
setup.c. I think changing the semantics for all callers was not necessary, and in fact, not desirable.
For one, setup_work_tree() prototype change is necessary to deal with (gitdir autodetection + core.worktree + setup_work_tree()).
For setup_*gently() changes, I think I can make setup_git_directory_gently() retain its current semantics by removing 06/10 and 07/10. But I do think it's nice to have setup_*_gently() does things gently _and_ easily-predictable (current _gently() setups worktree under certain conditions and not otherwise, which I consider uneasily-predictable). Patch 06 and 07 are mainly cleanup as a result from _gently() changes. Are those most intrusive to you?
Quoted 3 lines
Ciao,
Dscho
Signature · 1 line
Duy
PATCH 6/10 Completely move out worktree setup from setup_git_directory_gently()
In reply to a message not in the archiveThis was impossible earlier because git_dir can be relative. Now that git_dir is absolute, I see no reason for worktree setup inside setup_git_directory_gently(). The semantic is now clearer: if you need worktree, call setup_work_tree yourself (well, I will clean up setup_git_directory() part later)
This patch will free some commands from prefix handling if they don't ever need worktree.
--- builtin-apply.c | 7 +++++-- builtin-bundle.c | 9 ++------- builtin-config.c | 15 +++++---------- builtin-diff-files.c | 11 ++++++----- builtin-diff.c | 11 ++++++----- builtin-upload-archive.c | 4 ++-- cache.h | 2 +- git.c | 6 +----- hash-object.c | 8 +------- setup.c | 34 +++++++++++++++++----------------- 10 files changed, 46 insertions(+), 61 deletions(-)
builtin-apply.c +5 -2
@@ -2998,8 +2998,11 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)const char *whitespace_option =NULL ; - prefix =setup_git_directory_gently (&is_not_gitdir); - prefix_length = prefix ?strlen (prefix) :0 ; +setup_git_directory_gently (&is_not_gitdir); +if (!is_not_gitdir &&is_inside_work_tree ()) { + prefix =setup_work_tree (NULL ); + prefix_length = prefix ?strlen (prefix) :0 ; + }git_config (git_apply_config);if (apply_default_whitespace)parse_whitespace_option (apply_default_whitespace);
builtin-bundle.c +2 -7
@@ -11,7 +11,7 @@static const char *bundle_usage="git-bundle (create <bundle> <git-rev-list args> | verify <bundle> | list-heads <bundle> [refname]... | unbundle <bundle> [refname]... )" ; -int cmd_bundle (int argc,const char **argv,const char *prefix) +int cmd_bundle (int argc,const char **argv,const char *unused_prefix) {struct bundle_header header;int nongit =0 ;
@@ -27,12 +27,7 @@ int cmd_bundle(int argc, const char **argv, const char *prefix) argc -=2 ; argv +=2 ; - prefix =setup_git_directory_gently (&nongit); -if (prefix && bundle_file[0 ] !='/' ) { -snprintf (buffer,sizeof (buffer),"%s/%s" , prefix, bundle_file); - bundle_file = buffer; - } - +setup_git_directory_gently (&nongit);memset (&header,0 ,sizeof (header));if (strcmp (cmd,"create" ) && (bundle_fd =read_bundle_header (bundle_file, &header)) <0 )
builtin-config.c +5 -10
@@ -262,11 +262,11 @@ static int get_colorbool(int argc, const char **argv) } } -int cmd_config (int argc,const char **argv,const char *prefix) +int cmd_config (int argc,const char **argv,const char *unused_prefix) {int nongit =0 ;char * value; -const char *file =setup_git_directory_gently (&nongit); +setup_git_directory_gently (&nongit);while (1 < argc) {if (!strcmp (argv[1 ],"--int" ))
@@ -276,8 +276,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)else if (!strcmp (argv[1 ],"--list" ) || !strcmp (argv[1 ],"-l" )) {if (argc !=2 )usage (git_config_set_usage); -if (git_config (show_all_config) <0 && file && errno) -die ("unable to read config file %s: %s" , file, +if (git_config (show_all_config) <0 && errno) +die ("unable to read config file: %s" ,strerror (errno));return 0 ; }
@@ -296,12 +296,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)else if (!strcmp (argv[1 ],"--file" ) || !strcmp (argv[1 ],"-f" )) {if (argc <3 )usage (git_config_set_usage); -if (!is_absolute_path (argv[2 ]) && file) - file =prefix_filename (file,strlen (file), - argv[2 ]); -else - file = argv[2 ]; -setenv (CONFIG_ENVIRONMENT , file,1 ); +setenv (CONFIG_ENVIRONMENT , argv[2 ],1 ); argc--; argv++; }
builtin-diff-files.c +6 -5
@@ -13,21 +13,22 @@ static const char diff_files_usage[] ="git-diff-files [-q] [-0/-1/2/3 |-c|--cc|--no-index] [<common diff options>] [<path>...]" COMMON_DIFF_OPTIONS_HELP ; -int cmd_diff_files (int argc,const char **argv,const char *prefix) +int cmd_diff_files (int argc,const char **argv,const char *unused_prefix) {struct rev_info rev;int nongit =0 ;int result; - prefix =setup_git_directory_gently (&nongit); -init_revisions (&rev, prefix); +setup_git_directory_gently (&nongit); +init_revisions (&rev,NULL );git_config (git_diff_basic_config);/* no "diff" UI options */ rev.abbrev =0 ; -if (!setup_diff_no_index (&rev, argc, argv, nongit, prefix)) +if (!setup_diff_no_index (&rev, argc, argv, nongit,NULL )) argc =0 ;else { - rev.prefix =setup_work_tree (prefix); +init_revisions (&rev,setup_work_tree (NULL )); + rev.abbrev =0 ; argc =setup_revisions (argc, argv, &rev,NULL ); }if (!rev.diffopt.output_format)
builtin-diff.c +6 -5
@@ -202,7 +202,7 @@ static void refresh_index_quietly(void)rollback_lock_file (lock_file); } -int cmd_diff (int argc,const char **argv,const char *prefix) +int cmd_diff (int argc,const char **argv,const char *unused_prefix) {int i;struct rev_info rev;
@@ -233,19 +233,20 @@ int cmd_diff(int argc, const char **argv, const char *prefix)* Other cases are errors. */ - prefix =setup_git_directory_gently (&nongit); +setup_git_directory_gently (&nongit);git_config (git_diff_ui_config);if (diff_use_color_default ==-1 ) diff_use_color_default = git_use_color_default; -init_revisions (&rev, prefix); +init_revisions (&rev,NULL ); rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index; -if (!setup_diff_no_index (&rev, argc, argv, nongit, prefix)) +if (!setup_diff_no_index (&rev, argc, argv, nongit,NULL )) argc =0 ;else { - rev.prefix =setup_work_tree (prefix); +init_revisions (&rev,setup_work_tree (NULL )); + rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index; argc =setup_revisions (argc, argv, &rev,NULL ); }if (!rev.diffopt.output_format) {
builtin-upload-archive.c +2 -2
@@ -17,7 +17,7 @@ static const char lostchild[] ="git-upload-archive: archiver process was lost" ; -static int run_upload_archive (int argc,const char **argv,const char *prefix) +static int run_upload_archive (int argc,const char **argv,const char *unused_prefix) {struct archiver ar;const char *sent_argv[MAX_ARGS ];
@@ -67,7 +67,7 @@ static int run_upload_archive(int argc, const char **argv, const char *prefix)/* parse all options sent by the client */ treeish_idx =parse_archive_args (sent_argc, sent_argv, &ar); -parse_treeish_arg (sent_argv + treeish_idx, &ar.args, prefix); +parse_treeish_arg (sent_argv + treeish_idx, &ar.args,NULL );parse_pathspec_arg (sent_argv + treeish_idx +1 , &ar.args);return ar.write_archive (&ar.args);
cache.h +1 -1
@@ -316,7 +316,7 @@ extern const char *get_git_work_tree(void);extern const char **get_pathspec (const char *prefix,const char **pathspec);extern const char *setup_work_tree (const char *prefix); -extern const char *setup_git_directory_gently (int *); +extern void setup_git_directory_gently (int *);extern const char *setup_git_directory (void );extern const char *prefix_path (const char *prefix,int len,const char *path);extern const char *prefix_filename (const char *prefix,int len,const char *path);
git.c +1 -5
@@ -143,13 +143,12 @@ static int split_cmdline(char *cmdline, const char ***argv)static int handle_alias (int *argcp,const char ***argv) {int nongit =0 , envchanged =0 , ret =0 , saved_errno = errno; -const char *subdir;int count, option_count;const char ** new_argv;const char *alias_command;char *alias_string; - subdir =setup_git_directory_gently (&nongit); +setup_git_directory_gently (&nongit); alias_command = (*argv)[0 ]; alias_string =alias_lookup (alias_command);
@@ -205,9 +204,6 @@ static int handle_alias(int *argcp, const char ***argv) ret =1 ; } -if (subdir) -chdir (subdir); - errno = saved_errno;return ret;
hash-object.c +1 -7
@@ -38,14 +38,11 @@ int main(int argc, char **argv)int i;const char *type = blob_type;int write_object =0 ; -const char *prefix =NULL ; -int prefix_length =-1 ;int no_more_flags =0 ;int hashstdin =0 ;int nongit =0 ; - prefix =setup_git_directory_gently (&nongit); - prefix_length = prefix ?strlen (prefix) :0 ; +setup_git_directory_gently (&nongit);git_config (git_default_config);for (i =1 ; i < argc; i++) {
@@ -80,9 +77,6 @@ int main(int argc, char **argv)hash_stdin (type, write_object); hashstdin =0 ; } -if (0 <= prefix_length) - arg =prefix_filename (prefix, prefix_length, - arg);hash_object (arg,type_from_string (type), write_object); no_more_flags =1 ; }
setup.c +11 -10
@@ -301,7 +301,7 @@ static int check_repository_format_gently(int *nongit_ok)* We cannot decide in this function whether we are in the work tree or * not, since the config can only be read _after_ this function was called. */ -const char *setup_git_directory_gently (int *nongit_ok) +void setup_git_directory_gently (int *nongit_ok) {const char *work_tree_env =getenv (GIT_WORK_TREE_ENVIRONMENT );static char cwd[PATH_MAX +1 ];
@@ -335,11 +335,11 @@ const char *setup_git_directory_gently(int *nongit_ok) inside_work_tree =1 ; }check_repository_format_gently (nongit_ok); -return NULL ; +return ; }if (nongit_ok) { *nongit_ok =1 ; -return NULL ; +return ; }die ("Not a git repository: '%s'" , gitdirenv); }
@@ -364,9 +364,12 @@ const char *setup_git_directory_gently(int *nongit_ok) inside_git_dir =1 ;if (!work_tree_env) inside_work_tree =0 ; -setenv (GIT_DIR_ENVIRONMENT ,"." ,1 ); +if (chdir (cwd)) +die ("Cannot come back to cwd" ); + cwd[offset] ='\0' ; +setenv (GIT_DIR_ENVIRONMENT , cwd,1 );check_repository_format_gently (nongit_ok); -return NULL ; +return ; }chdir (".." ); do {
@@ -375,27 +378,23 @@ const char *setup_git_directory_gently(int *nongit_ok)if (chdir (cwd))die ("Cannot come back to cwd" ); *nongit_ok =1 ; -return NULL ; +return ; }die ("Not a git repository" ); } }while (cwd[--offset] !='/' ); } +if (chdir (cwd)) +die ("Cannot come back to cwd" ); inside_git_dir =0 ;if (!work_tree_env) inside_work_tree =1 ; git_work_tree_cfg =xstrndup (cwd, offset); -if (check_repository_format_gently (nongit_ok)) -return NULL ; -if (offset == len) -return NULL ;
Re: [PATCH 06/10] Completely move out worktree setup from setup_git_directory_gently()
Hi,
Quoted 5 lines · Nguyễn Thái Ngọc Duy
This was impossible earlier because
git_dircan be relative. Now thatgit_diris absolute, I see no reason for worktree setup insidesetup_git_directory_gently(). The semantic is now clearer: if you need worktree, callsetup_work_treeyourself (well, I will clean upsetup_git_directory()part later)
As I said earlier, the work for getting the prefix as most likely be done already in the search for .git/. I mean, it _is_ the common case to have a working tree with a .git/ in it, and that's it.
So I am quite certain that it is not worth the complicated and intrusive patch to separate the logic.
Particularly since working tree has a bad reputation already, as Junio pointed out: whenever we touch it, we get burnt.
Ciao,
Dscho
Re: [PATCH 06/10] Completely move out worktree setup from setup_git_directory_gently()
Quoted 11 lines · Johannes Schindelin
Hi,
Quoted 5 lines · Nguyễn Thái Ngọc Duy
This was impossible earlier because
git_dircan be relative. Now thatgit_diris absolute, I see no reason for worktree setup insidesetup_git_directory_gently(). The semantic is now clearer: if you need worktree, callsetup_work_treeyourself (well, I will clean upsetup_git_directory()part later)
Quoted 6 lines
As I said earlier, the work for getting the prefix as most likely be done already in the search for .git/. I mean, it _is_ the common case to have a working tree with a .git/ in it, and that's it.
So I am quite certain that it is not worth the complicated and intrusive patch to separate the logic.
It does for two purposes:
- Leave a chance for commands that does not care about worktree at
all. Those commands won't have to care about prefix anymore.
- Clean up the code. Now only setup_work_tree() does worktree setup
things (like chdir())
Quoted 2 lines
Particularly since working tree has a bad reputation already, as Junio pointed out: whenever we touch it, we get burnt.
I'd rather get burnt now than later (which might be probably worse) :) In case we are to be burnt, we should not have it in 1.5.5 :D
Quoted 2 lines
Ciao,
Dscho
Signature · 1 line
Duy
Re: [PATCH 06/10] Completely move out worktree setup from setup_git_directory_gently()
Hi,
Quoted 7 lines · Nguyen Thai Ngoc Duy
Quoted 2 lines · Johannes Schindelin
Particularly since working tree has a bad reputation already, as Junio pointed out: whenever we touch it, we get burnt.
I'd rather get burnt now than later (which might be probably worse) :) In case we are to be burnt, we should not have it in 1.5.5 :D
I'd rather be not burnt at all. That's why I am opposed to this huge (relative to what it accomplishes) patch series.
Ciao,
Dscho
PATCH 7/10 builtin-archive: mark unused prefix "unused_prefix"
In reply to a message not in the archivecmd_archive() is registered without RUN_SETUP so its prefix will be NULL forever. Let's make that clear.
--- builtin-archive.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
builtin-archive.c +3 -3
@@ -236,11 +236,12 @@ static const char *extract_remote_arg(int *ac, const char **av)return remote; } -int cmd_archive (int argc,const char **argv,const char *prefix) +int cmd_archive (int argc,const char **argv,const char *unused_prefix) {struct archiver ar;int tree_idx;const char *remote =NULL ; +const char *prefix; remote =extract_remote_arg (&argc, argv);if (remote)
@@ -250,9 +251,8 @@ int cmd_archive(int argc, const char **argv, const char *prefix)memset (&ar,0 ,sizeof (ar)); tree_idx =parse_archive_args (argc, argv, &ar); -if (prefix ==NULL ) - prefix =setup_git_directory (); + prefix =setup_git_directory (); argv += tree_idx;parse_treeish_arg (argv, &ar.args, prefix);parse_pathspec_arg (argv +1 , &ar.args);
Signature · 1 line
1.5.4.2.281.g28d0e
Re: [PATCH 07/10] builtin-archive: mark unused prefix "unused_prefix"
Hi,
Quoted 5 lines · Nguyễn Thái Ngọc Duy
cmd_archive()is registered withoutRUN_SETUPso its prefix will be NULL forever. Let's make that clear.Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>---
AFAICT this patch is -- like two others -- an independent fix, and should be submitted as such.
Ciao,
Dscho
Re: [PATCH 07/10] builtin-archive: mark unused prefix "unused_prefix"
Quoted 5 lines · Johannes Schindelin
Hi,
On Sun, 2 Mar 2008, Nguyễn Thái Ngọc Duy wrote:
Quoted 8 lines
Quoted 5 lines
cmd_archive()is registered withoutRUN_SETUPso its prefix will be NULL forever. Let's make that clear.Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>---
AFAICT this patch is -- like two others -- an independent fix, and should be submitted as such.
It's not really independent. Before the series, setup_dir_gently() _can_ change the prefix. After the series it does not (semantics change). The first patch of the series ("git read-tree -m") is independent and can be cherry picked if necessary.
Signature · 1 line
Duy
Re: [PATCH 07/10] builtin-archive: mark unused prefix "unused_prefix"
I hate to make this request, but Gmail seems unable to decode messages which are "Content-Disposition: inline" + "Content-Transfer-Encoding:
base64". The message body shows up completely blank in the Gmail web interface. (I wonder if it is due to the footer that Majordomo appends?)
I've reported this is a bug to Gmail, but since I imagine I'm not the only one using Gmail to read this list, and since so far your messages seem to be the only ones sent this way, can I ask that you configure your MUA to not send messages this way, if possible?
Much appreciated,
j.
Re: [PATCH 07/10] builtin-archive: mark unused prefix "unused_prefix"
Quoted 4 lines · Jay Soffian
I hate to make this request, but Gmail seems unable to decode messages
which are "Content-Disposition: inline" + "Content-Transfer-Encoding:
base64". The message body shows up completely blank in the Gmail web interface. (I wonder if it is due to the footer that Majordomo appends?)
Funny that meesage was created by gmail.
Quoted 4 lines
I've reported this is a bug to Gmail, but since I imagine I'm not the only one using Gmail to read this list, and since so far your messages seem to be the only ones sent this way, can I ask that you configure your MUA to not send messages this way, if possible?
If I does not misunderstand it, non-ascii characters make gmail encode messages base64. I'll be careful next time removing all non-ascii (or switch back to mutt).
Quoted 4 lines
Much appreciated,
j.
Signature · 1 line
Duy
PATCH 8/10 Make setup_git_directory() auto-setup worktree if found
In reply to a message not in the archiveThe semantic is simpler: if worktree is found, use it or die.
--- builtin-ls-files.c | 4 ++-- builtin-rev-parse.c | 7 +++++-- builtin-rm.c | 5 ++--- setup.c | 2 +- 4 files changed, 10 insertions(+), 8 deletions(-)
builtin-ls-files.c +2 -2
@@ -566,8 +566,8 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)break ; } -if (require_work_tree && !is_inside_work_tree ()) - prefix =setup_work_tree (prefix); +if (require_work_tree && !get_git_work_tree ()) +die ("This operation must be run in a work tree" );if (prefix_offset ==-1 ) prefix_offset = prefix ?strlen (prefix) :0 ;
builtin-rev-parse.c +5 -2
@@ -359,15 +359,16 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)return 0 ; } -int cmd_rev_parse (int argc,const char **argv,const char *prefix) +int cmd_rev_parse (int argc,const char **argv,const char *unused_prefix) {int i, as_is =0 , verify =0 ;unsigned char sha1[20 ]; +const char *prefix =NULL ;if (argc >1 && !strcmp ("--parseopt" , argv[1 ]))return cmd_parseopt (argc -1 , argv +1 , prefix); - prefix =setup_git_directory (); +setup_git_directory_gently (NULL );git_config (git_default_config);for (i =1 ; i < argc; i++) {const char *arg = argv[i];
@@ -487,6 +488,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)printf ("%s\n" , work_tree);continue ; } +else + pfx = prefix =setup_work_tree (prefix);while (pfx) { pfx =strchr (pfx,'/' );if (pfx) {
builtin-rm.c +0 -2
@@ -155,10 +155,9 @@ int cmd_rm(int argc, const char **argv, const char *prefix)if (!argc)usage_with_options (builtin_rm_usage, builtin_rm_options); -if (!index_only) - prefix =setup_work_tree (prefix);
PATCH 9/10 Documentation: update api-builtin and api-setup
In reply to a message not in the archive--- Documentation/technical/api-builtin.txt | 10 ++++ Documentation/technical/api-setup.txt | 91 ++++++++++++++++++++++++++++--- 2 files changed, 93 insertions(+), 8 deletions(-)
Documentation/technical/api-builtin.txt +10 -0
@@ -33,6 +33,10 @@ git: If the standard output is connected to a tty, spawn a pager and feed our output to it. +`NEED_WORK_TREE`:: + + Make sure there is a work tree to work on. + . Add `builtin-foo.o` to `BUILTIN_OBJS` in `Makefile`. Additionally, if `foo` is a new command, there are 3 more things to do:
@@ -59,5 +63,11 @@ to the subdirectory the command started from. This allows you to convert a user-supplied pathname (typically relative to that directory) to a pathname relative to the top of the work tree. +`NEED_WORK_TREE` also set `prefix` like `RUN_SETUP`. But it will `die()` +if there is no work tree. + +If neither `NEED_WORK_TREE` nor `RUN_SETUP` is set, `prefix` is always `NULL`. +No chdir(2) will be done. + The return value from `cmd_foo()` becomes the exit status of the command.
Documentation/technical/api-setup.txt +83 -8
96 lines not on this page · raw message
Signature · 1 line
1.5.4.2.281.g28d0e
PATCH 10/10 Additional tests to capture worktree special cases
In reply to a message not in the archiveMost of them are for setup_git_directory_gently() commands as those are likely to break.
--- t/t1501-worktree.sh | 57 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 55 insertions(+), 2 deletions(-)
t/t1501-worktree.sh +55 -2
@@ -47,7 +47,19 @@ export GIT_CONFIG="$(pwd)"/$GIT_DIR/config test_rev_parse 'subdirectory' false false true sub/dir/ cd ../../.. || exit 1 -say "core.worktree = absolute path" +say "core.worktree = absolute path with GIT_DIR unset" +mkdir -p $(pwd)/repo.git/work/sub/dir || exit 1 +unset GIT_DIR +export GIT_CONFIG=$(pwd)/repo.git/config +git config core.worktree "$(pwd)/repo.git/work" +test_rev_parse 'outside' false false false +cd repo.git/work || exit 1 +test_rev_parse 'inside' false true true '' +cd sub/dir || exit 1 +test_rev_parse 'subdirectory' false true true sub/dir/ +cd ../../../.. || exit 1 + +say "core.worktree = absolute path with GIT_DIR set" export GIT_DIR=$(pwd)/repo.git export GIT_CONFIG=$GIT_DIR/config git config core.worktree "$(pwd)/work"
@@ -58,6 +70,47 @@ cd sub/dir || exit 1 test_rev_parse 'subdirectory' false false true sub/dir/ cd ../../.. || exit 1 +test_expect_success '"git ls-files -o" gets correct prefix' ' + (cd work/sub && touch untracked && + test "$(git ls-files -o)" = untracked)' + +rm work/sub/untracked || exit 1 + +cat <<EOF >expected +:100644 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0000000000000000000000000000000000000000 M sub/tracked +EOF + +cat <<EOF >expected2 +:100644 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0000000000000000000000000000000000000000 M tracked +EOF + +test_expect_success '"git diff-files" gets correct prefix' ' + (cd work/sub && touch tracked && + git add tracked && echo modified > tracked && + git diff-files > ../../result && + git diff-files --relative > ../../result2 && + git rm -f tracked) && + cmp result expected && + cmp result2 expected2' + +cat <<EOF >expected +:100644 100644 e69de29... 0000000... M sub/tracked +EOF + +cat <<EOF >expected2 +:100644 100644 e69de29... 0000000... M tracked +EOF + + +test_expect_success '"git diff" gets correct prefix' ' + (cd work/sub && touch tracked && + git add tracked && echo modified > tracked && + git diff --raw > ../../result && + git diff --raw --relative > ../../result2 && + git rm -f tracked) && + cmp result expected && + cmp result2 expected2' + say "GIT_WORK_TREE=relative path (override core.worktree)" export GIT_DIR=$(pwd)/repo.git export GIT_CONFIG=$GIT_DIR/config
@@ -103,7 +156,7 @@ test_expect_success 'repo finds its work tree from work tree, too' ' test sub/dir/tracked = "$(git ls-files)") ' -test_expect_success '_gently() groks relative GIT_DIR & GIT_WORK_TREE' ' +test_expect_success '"git diff" setup worktree properly' ' cd repo.git/work/sub/dir && GIT_DIR=../../.. GIT_WORK_TREE=../.. GIT_PAGER= \ git diff --exit-code tracked &&
Signature · 1 line
1.5.4.2.281.g28d0e