something fishy with Git commit and log from file

something fishy with Git commit and log from file

[Pascal Obry]

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

Re: something fishy with Git commit and log from file

[Luciano Rocha]

[-- Attachment #1: Type: text/plain, Size: 1333 bytes --]

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 <luciano@eurotux.com>
Eurotux Informática, S.A. <http://www.eurotux.com/>

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: something fishy with Git commit and log from file

[Pascal Obry]

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

Re: something fishy with Git commit and log from file

[Junio C Hamano]

Pascal Obry <pascal@obry.net> 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".

Re: something fishy with Git commit and log from file

[Pascal Obry]

Junio C Hamano a écrit :
> Pascal Obry <pascal@obry.net> 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

Re: something fishy with Git commit and log from file

[Junio C Hamano]

Pascal Obry <pascal@obry.net> writes:

> Junio C Hamano a écrit :
>> Pascal Obry <pascal@obry.net> 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";

Re: something fishy with Git commit and log from file

[Pascal Obry]

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

[PATCH] files given on the command line are relative to $cwd

[Junio C Hamano]

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 <gitster@pobox.com>
---
 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 <n> 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

Re: [PATCH] files given on the command line are relative to $cwd

[Olivier Marin]

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.

Re: [PATCH] files given on the command line are relative to $cwd

[Junio C Hamano]

Olivier Marin <dkr+ml.git@free.fr> 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.

Re: [PATCH] files given on the command line are relative to $cwd

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 828 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] files given on the command line are relative to $cwd

[Junio C Hamano]

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

> Olivier Marin <dkr+ml.git@free.fr> 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

Re: [PATCH] files given on the command line are relative to $cwd

[Samuel Tardieu]

>>>>> "Junio" == Junio C Hamano <gitster@pobox.com> 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/

Re: [PATCH] files given on the command line are relative to $cwd

[Luciano Rocha]

[-- Attachment #1: Type: text/plain, Size: 842 bytes --]

On Thu, Aug 07, 2008 at 10:45:21AM +0200, Samuel Tardieu wrote:
> >>>>> "Junio" == Junio C Hamano <gitster@pobox.com> 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 <luciano@eurotux.com>
Eurotux Informática, S.A. <http://www.eurotux.com/>

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] files given on the command line are relative to $cwd

[Junio C Hamano]

Luciano Rocha <luciano@eurotux.com> writes:

> On Thu, Aug 07, 2008 at 10:45:21AM +0200, Samuel Tardieu wrote:
>> >>>>> "Junio" == Junio C Hamano <gitster@pobox.com> 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.