# something fishy with Git commit and log from file 15 messages from 2008-08-06 to 2008-08-07. Participants: Pascal Obry, Luciano Rocha, Junio C Hamano, Olivier Marin, Pierre Habouzit, Samuel Tardieu. Thread: https://gitlist.dev/t/14860 ## Pascal Obry, 2008-08-06 10:30 Subject: something fishy with Git commit and log from file Message-ID: <48997D2E.9030708@obry.net> URL: https://gitlist.dev/e/48997D2E.9030708%40obry.net ``` The following command fails on my repository: $ git commit --file=clog fatal: could not read log file 'clog': No such file or directory $ cat clog toto Using the following command the commit pass without problem: $ cat clog | git commit --file=- With GDB I get: $ gdb --args git ci --file=clog This GDB was configured as "i686-pc-cygwin"... (gdb) break strbuf_read_file Breakpoint 1 at 0x44e64a: file strbuf.c, line 301. (gdb) run Starting program: /usr/local/bin/git.exe ci --file=clog Breakpoint 1, strbuf_read_file (sb=0x22cac0, path=0x22ccfa "clog", hint=0) at strbuf.c:301 301 fd = open(path, O_RDONLY); (gdb) print path $1 = 0x0 ???? outch, this is strange, or a gdb artifact? (gdb) n 302 if (fd < 0) (gdb) print fd $2 = -1 (gdb) print *0x22ccfa $3 = 1735355491 (gdb) print (char)*0x22ccfa $4 = 99 'c' (gdb) print (char)*0x22ccfb $5 = 108 'l' (gdb) print (char)*0x22ccfc $6 = 111 'o' (gdb) print (char)*0x22ccfd $7 = 103 'g' (gdb) print (char)*0x22ccfe $8 = 0 '\0' But looks like path really contains 'clog'!!! No luck for now to find the problem. Note that the same command pass fine on other repositories. At this point this looks really like some kind of memory corruption... I'm on Windows, using Git for Cygwin and the compiler is: $ gcc --version gcc (GCC) 3.4.4 (cygming special, gdc 0.12, using dmd 0.125) Pascal. -- --|------------------------------------------------------ --| Pascal Obry Team-Ada Member --| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE --|------------------------------------------------------ --| http://www.obry.net --| "The best way to travel is by means of imagination" --| --| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595 ``` ## Luciano Rocha, 2008-08-06 10:44 Subject: Re: something fishy with Git commit and log from file Message-ID: <20080806104432.GG7121@bit.office.eurotux.com> URL: https://gitlist.dev/e/20080806104432.GG7121%40bit.office.eurotux.com In-Reply-To: <48997D2E.9030708@obry.net> ``` On Wed, Aug 06, 2008 at 12:30:06PM +0200, Pascal Obry wrote: > > The following command fails on my repository: > > $ git commit --file=clog > fatal: could not read log file 'clog': No such file or directory > > $ cat clog > toto > > Using the following command the commit pass without problem: > > $ cat clog | git commit --file=- > > With GDB I get: > > $ gdb --args git ci --file=clog > This GDB was configured as "i686-pc-cygwin"... > (gdb) break strbuf_read_file > Breakpoint 1 at 0x44e64a: file strbuf.c, line 301. > (gdb) run > Starting program: /usr/local/bin/git.exe ci --file=clog > > Breakpoint 1, strbuf_read_file (sb=0x22cac0, path=0x22ccfa "clog", hint=0) > at strbuf.c:301 > 301 fd = open(path, O_RDONLY); > (gdb) print path > $1 = 0x0 > > ???? outch, this is strange, or a gdb artifact? gdb artifact. The breakpoint info shows the correct value. Could you at that point run the following? (gdb) p get_current_dir_name() $1 = ... "..." If the returned value doesn't match the directory you were in, then there's a bug in git. If the function doesn't exist, try: (gdb) p getwd(malloc(2048)) Regards, Luciano Rocha -- Luciano Rocha Eurotux Informática, S.A. ``` ## Pascal Obry, 2008-08-06 11:01 Subject: Re: something fishy with Git commit and log from file Message-ID: <4899848C.6030800@obry.net> URL: https://gitlist.dev/e/4899848C.6030800%40obry.net In-Reply-To: <20080806104432.GG7121@bit.office.eurotux.com> ``` Luciano, > If the function doesn't exist, try: > (gdb) p getwd(malloc(2048)) I got: (gdb) p (char *)getwd(malloc(2048)) $3 = 0x903680 "/home/obry/dev/repositories/git/AWS" And this is not the directory I was in. It is the Git root. I was under /home/obry/dev/repositories/git/AWS/regtests when running the command. So definitely a Git bug! Can be reproduced with: $ mkdir repo && cd repo $ git init $ mkdir dir $ cd dir $ echo file > file $ echo log > log $ git add file $ git commit --file=log fatal: could not read log file 'log': No such file or directory Pascal. -- --|------------------------------------------------------ --| Pascal Obry Team-Ada Member --| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE --|------------------------------------------------------ --| http://www.obry.net --| "The best way to travel is by means of imagination" --| --| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595 ``` ## Junio C Hamano, 2008-08-06 15:38 Subject: Re: something fishy with Git commit and log from file Message-ID: <7v3alirw6b.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v3alirw6b.fsf%40gitster.siamese.dyndns.org In-Reply-To: <4899848C.6030800@obry.net> ``` Pascal Obry writes: > So definitely a Git bug! Can be reproduced with: > > $ mkdir repo && cd repo > $ git init > $ mkdir dir > $ cd dir > $ echo file > file > $ echo log > log > $ git add file > $ git commit --file=log > fatal: could not read log file 'log': No such file or directory Try it without cding down to "dir". ``` ## Pascal Obry, 2008-08-06 16:28 Subject: Re: something fishy with Git commit and log from file Message-ID: <4899D119.1080403@obry.net> URL: https://gitlist.dev/e/4899D119.1080403%40obry.net In-Reply-To: <7v3alirw6b.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano a écrit : > Pascal Obry writes: > >> So definitely a Git bug! Can be reproduced with: >> >> $ mkdir repo && cd repo >> $ git init >> $ mkdir dir >> $ cd dir >> $ echo file > file >> $ echo log > log >> $ git add file >> $ git commit --file=log >> fatal: could not read log file 'log': No such file or directory > > Try it without cding down to "dir". Yes it works. It also works if I do: $ echo log > ../log instead of $ echo log > log Git is looking for the log file at the Git root. Pascal. -- --|------------------------------------------------------ --| Pascal Obry Team-Ada Member --| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE --|------------------------------------------------------ --| http://www.obry.net --| "The best way to travel is by means of imagination" --| --| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595 ``` ## Junio C Hamano, 2008-08-06 16:50 Subject: Re: something fishy with Git commit and log from file Message-ID: <7vy73aqe9m.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vy73aqe9m.fsf%40gitster.siamese.dyndns.org In-Reply-To: <4899D119.1080403@obry.net> ``` Pascal Obry writes: > Junio C Hamano a écrit : >> Pascal Obry writes: >> >>> So definitely a Git bug! Can be reproduced with: >>> >>> $ mkdir repo && cd repo >>> $ git init >>> $ mkdir dir >>> $ cd dir >>> $ echo file > file >>> $ echo log > log >>> $ git add file >>> $ git commit --file=log >>> fatal: could not read log file 'log': No such file or directory >> >> Try it without cding down to "dir". > > Yes it works. Perhaps something like this. This must be another one of those regressions introduced in C rewrite. diff --git a/builtin-commit.c b/builtin-commit.c index b783e6e..fcc9c59 100644 --- a/builtin-commit.c +++ b/builtin-commit.c @@ -469,7 +469,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix) die("could not read log from standard input"); hook_arg1 = "message"; } else if (logfile) { - if (strbuf_read_file(&sb, logfile, 0) < 0) + const char *lf = logfile; + if (prefix) + lf = prefix_filename(prefix, strlen(prefix), logfile); + if (strbuf_read_file(&sb, lf, 0) < 0) die("could not read log file '%s': %s", logfile, strerror(errno)); hook_arg1 = "message"; ``` ## Pascal Obry, 2008-08-06 17:08 Subject: Re: something fishy with Git commit and log from file Message-ID: <4899DA8B.3010105@obry.net> URL: https://gitlist.dev/e/4899DA8B.3010105%40obry.net In-Reply-To: <7vy73aqe9m.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano a écrit : > Perhaps something like this. This must be another one of those > regressions introduced in C rewrite. Works fine now. Thanks. Pascal. -- --|------------------------------------------------------ --| Pascal Obry Team-Ada Member --| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE --|------------------------------------------------------ --| http://www.obry.net --| "The best way to travel is by means of imagination" --| --| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595 ``` ## Junio C Hamano, 2008-08-06 18:43 Subject: [PATCH] files given on the command line are relative to $cwd Message-ID: <7vr692oufw.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr692oufw.fsf%40gitster.siamese.dyndns.org In-Reply-To: <7vy73aqe9m.fsf@gitster.siamese.dyndns.org> ``` When running "git commit -F file" and "git tag -F file" from a subdirectory, we should take it as relative to the directory we started from, not relative to the top-level directory. This adds a helper function "parse_options_fix_filename()" to make it more convenient to fix this class of issues. Ideally, parse_options() should support a new type of option, "OPT_FILENAME", to do this uniformly, but this patch is meant to go to 'maint' to fix it minimally. One thing to note is that value for "commit template file" that comes from the command line is taken as relative to $cwd just like other parameters, but when it comes from the configuration varilable 'commit.template', it is taken as relative to the working tree root as before. I think this difference actually is sensible (not that I particularly think commit.template itself is sensible). Signed-off-by: Junio C Hamano --- builtin-commit.c | 11 +++++++---- builtin-tag.c | 1 + parse-options.c | 12 ++++++++++++ parse-options.h | 2 ++ t/t7500-commit.sh | 11 +++++++++++ 5 files changed, 33 insertions(+), 4 deletions(-) diff --git a/builtin-commit.c b/builtin-commit.c index bcbea38..0c6d1f4 100644 --- a/builtin-commit.c +++ b/builtin-commit.c @@ -45,7 +45,7 @@ static enum { COMMIT_PARTIAL, } commit_style; -static char *logfile, *force_author; +static const char *logfile, *force_author; static const char *template_file; static char *edit_message, *use_message; static char *author_name, *author_email, *author_date; @@ -700,11 +700,14 @@ static int message_is_empty(struct strbuf *sb, int start) } static int parse_and_validate_options(int argc, const char *argv[], - const char * const usage[]) + const char * const usage[], + const char *prefix) { int f = 0; argc = parse_options(argc, argv, builtin_commit_options, usage, 0); + logfile = parse_options_fix_filename(prefix, logfile); + template_file = parse_options_fix_filename(prefix, template_file); if (logfile || message.len || use_message) use_editor = 0; @@ -814,7 +817,7 @@ int cmd_status(int argc, const char **argv, const char *prefix) if (wt_status_use_color == -1) wt_status_use_color = git_use_color_default; - argc = parse_and_validate_options(argc, argv, builtin_status_usage); + argc = parse_and_validate_options(argc, argv, builtin_status_usage, prefix); index_file = prepare_index(argc, argv, prefix); @@ -907,7 +910,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) git_config(git_commit_config, NULL); - argc = parse_and_validate_options(argc, argv, builtin_commit_usage); + argc = parse_and_validate_options(argc, argv, builtin_commit_usage, prefix); index_file = prepare_index(argc, argv, prefix); diff --git a/builtin-tag.c b/builtin-tag.c index 3c97c69..3f77ba9 100644 --- a/builtin-tag.c +++ b/builtin-tag.c @@ -411,6 +411,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) git_config(git_tag_config, NULL); argc = parse_options(argc, argv, options, git_tag_usage, 0); + msgfile = parse_options_fix_filename(prefix, msgfile); if (keyid) { sign = 1; diff --git a/parse-options.c b/parse-options.c index f8d52e2..d771bf4 100644 --- a/parse-options.c +++ b/parse-options.c @@ -425,3 +425,15 @@ int parse_opt_approxidate_cb(const struct option *opt, const char *arg, *(unsigned long *)(opt->value) = approxidate(arg); return 0; } + +/* + * This should really be OPTION_FILENAME type as a part of + * parse_options that take prefix to do this while parsing. + */ +extern const char *parse_options_fix_filename(const char *prefix, const char *file) +{ + if (!file || !prefix || is_absolute_path(file)) + return file; + return prefix_filename(prefix, strlen(prefix), file); +} + diff --git a/parse-options.h b/parse-options.h index 4ee443d..13ad158 100644 --- a/parse-options.h +++ b/parse-options.h @@ -123,4 +123,6 @@ extern int parse_opt_approxidate_cb(const struct option *, const char *, int); "use digits to display SHA-1s", \ PARSE_OPT_OPTARG, &parse_opt_abbrev_cb, 0 } +extern const char *parse_options_fix_filename(const char *prefix, const char *file); + #endif diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh index baed6ce..026d787 100755 --- a/t/t7500-commit.sh +++ b/t/t7500-commit.sh @@ -138,4 +138,15 @@ test_expect_success '--signoff' ' diff expect output ' +test_expect_success 'commit message from file' ' + mkdir subdir && + echo "Log in top directory" >log && + echo "Log in sub directory" >subdir/log && + ( + cd subdir && + git commit --allow-empty -F log + ) && + commit_msg_is "Log in sub directory" +' + test_done ``` ## Olivier Marin, 2008-08-06 20:14 Subject: Re: [PATCH] files given on the command line are relative to $cwd Message-ID: <489A061B.7010508@free.fr> URL: https://gitlist.dev/e/489A061B.7010508%40free.fr In-Reply-To: <7vr692oufw.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano a écrit : > > static int parse_and_validate_options(int argc, const char *argv[], > - const char * const usage[]) > + const char * const usage[], > + const char *prefix) > { > int f = 0; > > argc = parse_options(argc, argv, builtin_commit_options, usage, 0); > + logfile = parse_options_fix_filename(prefix, logfile); It breaks the "git commit -F -" case, no? Olivier. ``` ## Junio C Hamano, 2008-08-06 20:19 Subject: Re: [PATCH] files given on the command line are relative to $cwd Message-ID: <7vtzdxnbgk.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtzdxnbgk.fsf%40gitster.siamese.dyndns.org In-Reply-To: <489A061B.7010508@free.fr> ``` Olivier Marin writes: > Junio C Hamano a écrit : >> >> static int parse_and_validate_options(int argc, const char *argv[], >> - const char * const usage[]) >> + const char * const usage[], >> + const char *prefix) >> { >> int f = 0; >> >> argc = parse_options(argc, argv, builtin_commit_options, usage, 0); >> + logfile = parse_options_fix_filename(prefix, logfile); > > It breaks the "git commit -F -" case, no? Does it? Ah, yeah, t7500 #15 does not go down to a subdirectory. ``` ## Pierre Habouzit, 2008-08-06 20:40 Subject: Re: [PATCH] files given on the command line are relative to $cwd Message-ID: <20080806204002.GB903@artemis.madism.org> URL: https://gitlist.dev/e/20080806204002.GB903%40artemis.madism.org In-Reply-To: <7vr692oufw.fsf@gitster.siamese.dyndns.org> ``` On Wed, Aug 06, 2008 at 06:43:47PM +0000, Junio C Hamano wrote: > When running "git commit -F file" and "git tag -F file" from a > subdirectory, we should take it as relative to the directory we started > from, not relative to the top-level directory. > > This adds a helper function "parse_options_fix_filename()" to make it more > convenient to fix this class of issues. Ideally, parse_options() should > support a new type of option, "OPT_FILENAME", to do this uniformly, but > this patch is meant to go to 'maint' to fix it minimally. I'm going in vacation tomorrow so I'm not likely to do that soon, but I agree it's sensible. -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org ``` ## Junio C Hamano, 2008-08-06 20:59 Subject: Re: [PATCH] files given on the command line are relative to $cwd Message-ID: <7vd4kln9le.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vd4kln9le.fsf%40gitster.siamese.dyndns.org In-Reply-To: <7vtzdxnbgk.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano writes: > Olivier Marin writes: > >> Junio C Hamano a écrit : >>> >>> static int parse_and_validate_options(int argc, const char *argv[], >>> - const char * const usage[]) >>> + const char * const usage[], >>> + const char *prefix) >>> { >>> int f = 0; >>> >>> argc = parse_options(argc, argv, builtin_commit_options, usage, 0); >>> + logfile = parse_options_fix_filename(prefix, logfile); >> >> It breaks the "git commit -F -" case, no? > > Does it? Ah, yeah, t7500 #15 does not go down to a subdirectory. Ok, this squashed in on top of the previous one should cover the case. Thanks for saving me in time from a major embarrassment. I already tagged 1.5.6.5 with the botched one but haven't pushed it out, so I can safely rewind. --- parse-options.c | 2 +- t/t7500-commit.sh | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletions(-) diff --git a/parse-options.c b/parse-options.c index d771bf4..12c8822 100644 --- a/parse-options.c +++ b/parse-options.c @@ -432,7 +432,7 @@ int parse_opt_approxidate_cb(const struct option *opt, const char *arg, */ extern const char *parse_options_fix_filename(const char *prefix, const char *file) { - if (!file || !prefix || is_absolute_path(file)) + if (!file || !prefix || is_absolute_path(file) || !strcmp("-", file)) return file; return prefix_filename(prefix, strlen(prefix), file); } diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh index 2ab791b..823256a 100755 --- a/t/t7500-commit.sh +++ b/t/t7500-commit.sh @@ -159,4 +159,12 @@ test_expect_success 'commit message from file (2)' ' commit_msg_is "Log in sub directory" ' +test_expect_success 'commit message from stdin' ' + ( + cd subdir && + echo "Log with foo word" | git commit --allow-empty -F - + ) && + commit_msg_is "Log with foo word" +' + test_done ``` ## Samuel Tardieu, 2008-08-07 08:45 Subject: Re: [PATCH] files given on the command line are relative to $cwd Message-ID: <2008-08-07-10-45-21+trackit+sam@rfc1149.net> URL: https://gitlist.dev/e/2008-08-07-10-45-21%2Btrackit%2Bsam%40rfc1149.net In-Reply-To: <7vr692oufw.fsf@gitster.siamese.dyndns.org> ``` >>>>> "Junio" == Junio C Hamano writes: Junio> When running "git commit -F file" and "git tag -F file" from a Junio> subdirectory, we should take it as relative to the directory we Junio> started from, not relative to the top-level directory. Don't we have the same problem with "git show"? If you go into the "gitweb" directory of the GIT source, "git show HEAD:README" will show you the toplevel "README" instead of the one in the "gitweb" directory. Sam -- Samuel Tardieu -- sam@rfc1149.net -- http://www.rfc1149.net/ ``` ## Luciano Rocha, 2008-08-07 09:03 Subject: Re: [PATCH] files given on the command line are relative to $cwd Message-ID: <20080807090341.GA6421@bit.office.eurotux.com> URL: https://gitlist.dev/e/20080807090341.GA6421%40bit.office.eurotux.com In-Reply-To: <2008-08-07-10-45-21+trackit+sam@rfc1149.net> ``` On Thu, Aug 07, 2008 at 10:45:21AM +0200, Samuel Tardieu wrote: > >>>>> "Junio" == Junio C Hamano writes: > > Junio> When running "git commit -F file" and "git tag -F file" from a > Junio> subdirectory, we should take it as relative to the directory we > Junio> started from, not relative to the top-level directory. > > Don't we have the same problem with "git show"? If you go into > the "gitweb" directory of the GIT source, "git show HEAD:README" will > show you the toplevel "README" instead of the one in the "gitweb" > directory. No, git show has different semantics. It has been discussed often in this list. For instance: http://thread.gmane.org/gmane.comp.version-control.git/68786/focus=68852 -- Luciano Rocha Eurotux Informática, S.A. ``` ## Junio C Hamano, 2008-08-07 09:47 Subject: Re: [PATCH] files given on the command line are relative to $cwd Message-ID: <7v8wv9jgw4.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8wv9jgw4.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080807090341.GA6421@bit.office.eurotux.com> ``` Luciano Rocha writes: > On Thu, Aug 07, 2008 at 10:45:21AM +0200, Samuel Tardieu wrote: >> >>>>> "Junio" == Junio C Hamano writes: >> >> Junio> When running "git commit -F file" and "git tag -F file" from a >> Junio> subdirectory, we should take it as relative to the directory we >> Junio> started from, not relative to the top-level directory. >> >> Don't we have the same problem with "git show"? If you go into >> the "gitweb" directory of the GIT source, "git show HEAD:README" will >> show you the toplevel "README" instead of the one in the "gitweb" >> directory. > > No, git show has different semantics. It has been discussed often in > this list. You are half correct --- it is not show but tree:path syntax. ```