threads / patch / 27795

patch, 3 partsdoc/{fast-import,remote-helpers}: few clarifications

Subject: [PATCH 0/3] doc/{fast-import,remote-helpers}: few clarifications

## tl;dr

14 messages between Jul 12, 2011 and Jul 22, 2011. Diffs are folded; open one to read it.

replies: 13people: 5as markdown or json

Dmitry Ivankov· Jul 12, 2011, 15:10 UTC · lore

doc/fast-import part seems pretty clear. doc/remote-helpers is related to a recent remote-helpers series [1] and may need some discussion. For example, should we mention a bug [2] addressed as a part of [1], should we wait for [1] to document the fact that gitdir response can be read in the middle of capabilities reporting.

[1] http://thread.gmane.org/gmane.comp.version-control.git/176002 [2] http://thread.gmane.org/gmane.comp.version-control.git/176002/focus=176020

Dmitry Ivankov (3):
  doc/fast-import: clarify notemodify command
  doc/fast-import: document feature import-marks-if-exists
  doc/remote-helpers: document the gitdir feature
 Documentation/git-fast-import.txt    |   16 +++++++++++++---
 Documentation/git-remote-helpers.txt |   15 +++++++++++++++
 t/t9300-fast-import.sh               |   15 +++++++++++++++
 3 files changed, 43 insertions(+), 3 deletions(-)
-- 
1.7.3.4
Dmitry Ivankov· Jul 12, 2011, 15:10 UTC · re: Dmitry Ivankov · lore

[PATCH 1/3] doc/fast-import: clarify notemodify command

The "notemodify" fast-import command was introduced in commit a8dd2.. The commit log has slightly different description than the added documentation. The latter is somewhat confusing. "notemodify" is a subcommand of "commit" command used to add a note for some commit. Does this note annotate the commit produced by the "commit" command or a commit given by it's committish parameter? Which notes tree does it write notes to?

The exact meaning could be deduced with old description and some
notes machinery knowledge. But let's make it more obvious. This
command is used in a context like "commit refs/notes/test" to
add or rewrite an annotation for a committish parameter. So the
advised way to add notes in a fast-import stream is:
1) import some commits (optional)
2) prepare a "commit" to the notes tree:
2.1) choose notes ref, committer, log message, etc.
2.2) create annotations with "notemodify", where each can refer to
a commit being annotated via a branch name, import mark reference,
sha1 and other expressions specified in the Documentation.
Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 Documentation/git-fast-import.txt |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)
Show changes to Documentation/git-fast-import.txt +8 −3
diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 249249a..3f5b912 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -648,9 +648,14 @@ paths for a commit are encouraged to do so.
 
 `notemodify`
 ^^^^^^^^^^^^
-Included in a `commit` command to add a new note (annotating a given
-commit) or change the content of an existing note.  This command has
-two different means of specifying the content of the note.
+Included in a `commit` `<notes_ref>` command to add a new note
+annotating a `<committish>` or change this annotation contents.
+Internally it is similar to filemodify 100644 on `<committish>`
+path (maybe split into subdirectories). It's not advised to
+use any other commands to write to the `<notes_ref>` tree except
+`filedeleteall` to delete all existing notes in this tree.
+This command has two different means of specifying the content
+of the note.
 
 External data format::
 	The data content for the note was already supplied by a prior
-- 
1.7.3.4
Dmitry Ivankov· Jul 12, 2011, 15:10 UTC · re: Dmitry Ivankov · lore

[PATCH 2/3] doc/fast-import: document feature import-marks-if-exists

fast-import parameter --import-marks-if-exists was introduced in commit dded4.. --import-marks can be set via a "feature" command in a fast-import stream and --import-marks-if-exists has support for such specification too, for free.

Document "feature import-marks-if-exists=<file>" command and add a minimalistic test for it.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 Documentation/git-fast-import.txt |    5 +++++
 t/t9300-fast-import.sh            |   15 +++++++++++++++
 2 files changed, 20 insertions(+), 0 deletions(-)
Show changes to 2 files +20 −0

Documentation/git-fast-import.txt, t/t9300-fast-import.sh

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 3f5b912..bdcc81c 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -1005,6 +1005,11 @@ import-marks::
 	second, an --import-marks= command-line option overrides
 	any "feature import-marks" command in the stream.
 
+import-marks-if-exists::
+	Like import-marks but instead of erroring out, silently
+	skips the file if it does not exist. Differences from --
+	version are the same as with import-marks feature above.
+
 cat-blob::
 ls::
 	Require that the backend support the 'cat-blob' or 'ls' command.
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 2a53640..eed57df 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -1882,6 +1882,21 @@ test_expect_success 'R: --import-marks-if-exists' '
 	test_cmp expect io.marks
 '
 
+test_expect_success 'R: feature import-marks-if-exists' '
+	rm -f io.marks &&
+	blob=$(echo hi | git hash-object --stdin) &&
+	echo ":1 $blob" >expect &&
+	git fast-import --import-marks-if-exists=io.marks --export-marks=io.marks <<-\EOF &&
+	feature import-marks-if-exists=io.marks
+	blob
+	mark :1
+	data 3
+	hi
+
+	EOF
+	test_cmp expect io.marks
+'
+
 cat >input << EOF
 feature import-marks=marks.out
 feature export-marks=marks.new
-- 
1.7.3.4
Junio C Hamano· Jul 12, 2011, 17:00 UTC · re: Dmitry Ivankov · lore

Re: [PATCH 2/3] doc/fast-import: document feature import-marks-if-exists

Dmitry Ivankov <divanorama@gmail.com> writes:
Show 9 quoted lines
> fast-import parameter --import-marks-if-exists was introduced in
> commit dded4.. --import-marks can be set via a "feature" command in
> a fast-import stream and --import-marks-if-exists has support for
> such specification too, for free.
>
> Document "feature import-marks-if-exists=<file>" command and add a
> minimalistic test for it.
>
> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
Thanks.
Show 16 quoted lines
>  Documentation/git-fast-import.txt |    5 +++++
>  t/t9300-fast-import.sh            |   15 +++++++++++++++
>  2 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> index 3f5b912..bdcc81c 100644
> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> @@ -1005,6 +1005,11 @@ import-marks::
>  	second, an --import-marks= command-line option overrides
>  	any "feature import-marks" command in the stream.
>  
> +import-marks-if-exists::
> +	Like import-marks but instead of erroring out, silently
> +	skips the file if it does not exist. Differences from --
> +	version are the same as with import-marks feature above.

Wouldn't it be more concise and complete to do it this way instead? It avoids duplication and also clarifies you cannot mix these two.

Show changes to Documentation/git-fast-import.txt +6 −2
diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 249249a..e939e74 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -995,10 +995,14 @@ force::
 	(see OPTIONS, above).
 
 import-marks::
+import-marks-if-exists::
 	Like --import-marks except in two respects: first, only one
-	"feature import-marks" command is allowed per stream;
+	"feature import-marks" command or "feature import-marks-if-exists"
+	is allowed per stream;
 	second, an --import-marks= command-line option overrides
-	any "feature import-marks" command in the stream.
+	any "feature import-marks" command in the stream. Similarly,
+	import-marks-if-exists corresponds to --import-marks-if-exists
+	from the command line.
 
 cat-blob::
 ls::

>  
> +test_expect_success 'R: feature import-marks-if-exists' '
> +	rm -f io.marks &&
> +	blob=$(echo hi | git hash-object --stdin) &&
> +	echo ":1 $blob" >expect &&
> +	git fast-import --import-marks-if-exists=io.marks --export-marks=io.marks <<-\EOF &&
> +	feature import-marks-if-exists=io.marks
> +	blob
> +	mark :1
> +	data 3
> +	hi
> +
> +	EOF
> +	test_cmp expect io.marks
> +'

Do you really want both the command line option and feature in this test?

There are at least four cases [*1*] you would want to test (a part of
blame lies in the original test you copied this from).

 - "if exists" should not fail if the marks file does not exist;
 - "if exists" reads marks if exists (not tested in this patch);
 - having "feature import-marks-if-exists" will trigger this feature
   without command line option (not tested in this patch); and
 - having "feature import-marks-if-exists" will be overridden by
   the command line option (not tested in this patch).




[Footnote]

*1* Ideally, the test should also cover these 16 combinations:

  io.marks file (2 cases: exists or missing)

times

  --import-mark-if-exists (2 cases: exists or missing)

times

 feature import-marks-if-exists (4 cases: missing,
 				same as command line,
                                different from command line,
				given more than one)
Ramkumar Ramachandra· Jul 13, 2011, 10:27 UTC · re: Dmitry Ivankov · lore

Re: [PATCH 2/3] doc/fast-import: document feature import-marks-if-exists

Hi Dmitry,
In addition to Junio's comments, I have a few things to add:
Dmitry Ivankov writes:
> fast-import parameter --import-marks-if-exists was introduced in
It's a command-line option, not a parameter.
> commit dded4.. --import-marks can be set via a "feature" command in

Please more SHA-1 hex characters while abbreviating, and write a little more information about the commit: dded4f1 (fast-import: Introduce --import-marks-if-exists, 2011-01-15)

Show 5 quoted lines
> a fast-import stream and --import-marks-if-exists has support for
> such specification too, for free.
>
> Document "feature import-marks-if-exists=<file>" command and add a
> minimalistic test for it.

Perhaps you can make it clearer that this feature was available back in dded4f1, and was supposed to have been documented then?

Thanks for working on this. I'm to blame for not documenting/ testing the feature in the original patch.

-- Ram
Dmitry Ivankov· Jul 12, 2011, 15:10 UTC · re: Dmitry Ivankov · lore

[PATCH 3/3] doc/remote-helpers: document the gitdir feature

The gitdir capability is recognized by git and can be used to tell the helper where the .git directory is. But it is not mentioned in the documentation.

Document the gitdir feature. Upon reading this capability transport-helper sends out "gitdir path_to_dot_git" command to the helper. Which looks exactly like sending a command. So let's call this a command.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 Documentation/git-remote-helpers.txt |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)
Show changes to Documentation/git-remote-helpers.txt +15 −0
diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
index 58f6ad4..a16a5e5 100644
--- a/Documentation/git-remote-helpers.txt
+++ b/Documentation/git-remote-helpers.txt
@@ -159,6 +159,14 @@ Supported if the helper has the "import" capability.
 +
 Supported if the helper has the "connect" capability.
 
+'gitdir' <path>::
+	Tells helper the location of current repository .git
+	directory. The path is absolute. The command is issued
+	immediately after "gitdir" capability line is read by
+	the caller.
++
+Supported if the helper has the "gitdir" capability.
+
 If a fatal error occurs, the program writes the error message to
 stderr and exits. The caller should expect that a suitable error
 message has been printed if the child closes the connection without
@@ -175,6 +183,7 @@ CAPABILITIES
 'push'::
 'import'::
 'connect'::
+'gitdir'::
 	This helper supports the corresponding command with the same name.
 
 'refspec' 'spec'::
@@ -187,6 +196,12 @@ CAPABILITIES
 	all, it must cover all refs reported by the list command; if
 	it is not used, it is effectively "{asterisk}:{asterisk}"
 
+'gitdir'::
+	The helper wants to know .git directory location. The gitdir
+	command is sent immediately after reading this capability.
+	The helper can read it immediately too or it can proceed with
+	reporting other capabilities and read it as a next command.
+
 REF LIST ATTRIBUTES
 -------------------
 
-- 
1.7.3.4
Sverre Rabbelier· Jul 13, 2011, 12:04 UTC · re: Dmitry Ivankov · lore

Re: [PATCH 3/3] doc/remote-helpers: document the gitdir feature

Heya,
On Tue, Jul 12, 2011 at 17:10, Dmitry Ivankov <divanorama@gmail.com> wrote:
Show 8 quoted lines
> The gitdir capability is recognized by git and can be used to tell
> the helper where the .git directory is. But it is not mentioned in
> the documentation.
>
> Document the gitdir feature. Upon reading this capability
> transport-helper sends out "gitdir path_to_dot_git" command to the
> helper. Which looks exactly like sending a command. So let's call
> this a command.

Johannes suggested that we should remove this feature and instead always export an GITDIR environmental variable. I'm inclined to agree.

-- 
Cheers,

Sverre Rabbelier
Dmitry Ivankov· Jul 13, 2011, 17:10 UTC · re: Dmitry Ivankov · lore

[PATCH v2 1/3] doc/fast-import: clarify notemodify command

The "notemodify" fast-import command was introduced in commit a8dd2e7 (fast-import: Add support for importing commit notes, 2009-10-09) The commit log has slightly different description than the added documentation. The latter is somewhat confusing. "notemodify" is a subcommand of "commit" command used to add a note for some commit. Does this note annotate the commit produced by the "commit" command or a commit given by it's committish parameter? Which notes tree does it write notes to?

The exact meaning could be deduced with old description and some
notes machinery knowledge. But let's make it more obvious. This
command is used in a context like "commit refs/notes/test" to
add or rewrite an annotation for a committish parameter. So the
advised way to add notes in a fast-import stream is:
1) import some commits (optional)
2) prepare a "commit" to the notes tree:
2.1) choose notes ref, committer, log message, etc.
2.2) create annotations with "notemodify", where each can refer to
a commit being annotated via a branch name, import mark reference,
sha1 and other expressions specified in the Documentation.
Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
Just added a bit more metadata of a8dd2e7.
 Documentation/git-fast-import.txt |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)
Show changes to Documentation/git-fast-import.txt +8 −3
diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 249249a..3f5b912 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -648,9 +648,14 @@ paths for a commit are encouraged to do so.
 
 `notemodify`
 ^^^^^^^^^^^^
-Included in a `commit` command to add a new note (annotating a given
-commit) or change the content of an existing note.  This command has
-two different means of specifying the content of the note.
+Included in a `commit` `<notes_ref>` command to add a new note
+annotating a `<committish>` or change this annotation contents.
+Internally it is similar to filemodify 100644 on `<committish>`
+path (maybe split into subdirectories). It's not advised to
+use any other commands to write to the `<notes_ref>` tree except
+`filedeleteall` to delete all existing notes in this tree.
+This command has two different means of specifying the content
+of the note.
 
 External data format::
 	The data content for the note was already supplied by a prior
-- 
1.7.3.4
Johan Herland· Jul 22, 2011, 19:32 UTC · re: Dmitry Ivankov · lore

Re: [PATCH v2 1/3] doc/fast-import: clarify notemodify command

On Wednesday 13 July 2011, Dmitry Ivankov wrote:
Show 22 quoted lines
> The "notemodify" fast-import command was introduced in commit a8dd2e7
> (fast-import: Add support for importing commit notes, 2009-10-09)
> The commit log has slightly different description than the added
> documentation. The latter is somewhat confusing. "notemodify" is a
> subcommand of "commit" command used to add a note for some commit.
> Does this note annotate the commit produced by the "commit" command
> or a commit given by it's committish parameter? Which notes tree
> does it write notes to?
> 
> The exact meaning could be deduced with old description and some
> notes machinery knowledge. But let's make it more obvious. This
> command is used in a context like "commit refs/notes/test" to
> add or rewrite an annotation for a committish parameter. So the
> advised way to add notes in a fast-import stream is:
> 1) import some commits (optional)
> 2) prepare a "commit" to the notes tree:
> 2.1) choose notes ref, committer, log message, etc.
> 2.2) create annotations with "notemodify", where each can refer to
> a commit being annotated via a branch name, import mark reference,
> sha1 and other expressions specified in the Documentation.
> 
> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
Acked-by: Johan Herland <johan@herland.net>
-- 
Johan Herland, <johan@herland.net>
www.herland.net
Dmitry Ivankov· Jul 13, 2011, 17:10 UTC · re: Dmitry Ivankov · lore

[PATCH v2 2/3] doc/fast-import: document feature import-marks-if-exists

fast-import command-line option --import-marks-if-exists was introduced in commit dded4f1 (fast-import: Introduce --import-marks-if-exists, 2011-01-15)

--import-marks option can be set via a "feature" command in a fast-import stream and --import-marks-if-exists had support for such specification from the very beginning too due to some shared codebase. Though the documentation for this feature wasn't written in dded4f1.

Add the documentation for "feature import-marks-if-exists=<file>". Also add a minimalistic test for it.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
Reworded a bit, united import-marks* descriptions. One more case to test:
--import-marks-if-exists= overrides feature import-marks=. And "fixed" the
test, I left the --import-marks accidentally, though it was a fine test too.
Also not feeling like adding a whole ton of coverage tests yet.
 Documentation/git-fast-import.txt |   10 +++++++---
 t/t9300-fast-import.sh            |   15 +++++++++++++++
 2 files changed, 22 insertions(+), 3 deletions(-)
Show changes to 2 files +22 −3

Documentation/git-fast-import.txt, t/t9300-fast-import.sh

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 3f5b912..3a5aa55 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -1000,10 +1000,14 @@ force::
 	(see OPTIONS, above).
 
 import-marks::
+import-marks-if-exists::
 	Like --import-marks except in two respects: first, only one
-	"feature import-marks" command is allowed per stream;
-	second, an --import-marks= command-line option overrides
-	any "feature import-marks" command in the stream.
+	"feature import-marks" or "feature import-marks-if-exists"
+	command is allowed per stream; second, an --import-marks=
+	or --import-marks-if-exists command-line option overrides
+	any of these "feature" commands in the stream; third,
+	"feature import-marks-if-exists" like a corresponding
+	command-line option silently skips a nonexistent file.
 
 cat-blob::
 ls::
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 2a53640..9af1a78 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -1882,6 +1882,21 @@ test_expect_success 'R: --import-marks-if-exists' '
 	test_cmp expect io.marks
 '
 
+test_expect_success 'R: feature import-marks-if-exists' '
+	rm -f io.marks &&
+	blob=$(echo hi | git hash-object --stdin) &&
+	echo ":1 $blob" >expect &&
+	git fast-import --export-marks=io.marks <<-\EOF &&
+	feature import-marks-if-exists=io.marks
+	blob
+	mark :1
+	data 3
+	hi
+
+	EOF
+	test_cmp expect io.marks
+'
+
 cat >input << EOF
 feature import-marks=marks.out
 feature export-marks=marks.new
-- 
1.7.3.4
Dmitry Ivankov· Jul 13, 2011, 17:10 UTC · re: Dmitry Ivankov · lore

[PATCH v2 3/3] remote-helpers: export GIT_DIR variable to helpers

The gitdir capability is recognized by git and can be used to tell the helper where the .git directory is. But it is not mentioned in the documentation and considered worse than if gitdir was passed via GIT_DIR environment variable.

Do export GIT_DIR for a remote helper. Switch testgit to use env instead of less favoured capability gitdir.

Mention it's possible uses in documentation: data storage, auxiliary git commands needed by a helper.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
Instead of documenting capability gitdir export GIT_DIR. Turned out
to be quite straightforward.
 Documentation/git-remote-helpers.txt |   18 ++++++++++++++++++
 git-remote-testgit.py                |   14 +-------------
 transport-helper.c                   |   10 ++++++++++
 3 files changed, 29 insertions(+), 13 deletions(-)
Show changes to 3 files +29 −13

Documentation/git-remote-helpers.txt, git-remote-testgit.py, transport-helper.c

diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
index 58f6ad4..394fc8e 100644
--- a/Documentation/git-remote-helpers.txt
+++ b/Documentation/git-remote-helpers.txt
@@ -47,6 +47,9 @@ arguments. The first argument specifies a remote repository as in git;
 it is either the name of a configured remote or a URL. The second
 argument specifies a URL; it is usually of the form
 '<transport>://<address>', but any arbitrary string is possible.
+'GIT_DIR' environment variable is set up for the remote helper and
+can be used to store some additional data or to invoke auxiliary git
+commands.
 
 When git encounters a URL of the form '<transport>://<address>', where
 '<transport>' is a protocol that it cannot handle natively, it
@@ -159,6 +162,14 @@ Supported if the helper has the "import" capability.
 +
 Supported if the helper has the "connect" capability.
 
+'gitdir' <path>::
+	Tells helper the location of current repository .git
+	directory. The path is absolute. The command is issued
+	immediately after "gitdir" capability line is read by
+	the caller.
++
+Supported if the helper has the "gitdir" capability.
+
 If a fatal error occurs, the program writes the error message to
 stderr and exits. The caller should expect that a suitable error
 message has been printed if the child closes the connection without
@@ -175,6 +186,7 @@ CAPABILITIES
 'push'::
 'import'::
 'connect'::
+'gitdir'::
 	This helper supports the corresponding command with the same name.
 
 'refspec' 'spec'::
@@ -187,6 +199,12 @@ CAPABILITIES
 	all, it must cover all refs reported by the list command; if
 	it is not used, it is effectively "{asterisk}:{asterisk}"
 
+'gitdir'::
+	The helper wants to know .git directory location. The gitdir
+	command is sent immediately after reading this capability.
+	The helper can read it immediately too or it can proceed with
+	reporting other capabilities and read it as a next command.
+
 REF LIST ATTRIBUTES
 -------------------
 
diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index df9d512..df3e37f 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -35,7 +35,7 @@ def get_repo(alias, url):
     prefix = 'refs/testgit/%s/' % alias
     debug("prefix: '%s'", prefix)
 
-    repo.gitdir = ""
+    repo.gitdir = os.environ["GIT_DIR"]
     repo.alias = alias
     repo.prefix = prefix
 
@@ -70,7 +70,6 @@ def do_capabilities(repo, args):
 
     print "import"
     print "export"
-    print "gitdir"
     print "refspec refs/heads/*:%s*" % repo.prefix
 
     print # end capabilities
@@ -150,22 +149,11 @@ def do_export(repo, args):
     repo.non_local.push(repo.gitdir)
 
 
-def do_gitdir(repo, args):
-    """Stores the location of the gitdir.
-    """
-
-    if not args:
-        die("gitdir needs an argument")
-
-    repo.gitdir = ' '.join(args)
-
-
 COMMANDS = {
     'capabilities': do_capabilities,
     'list': do_list,
     'import': do_import,
     'export': do_export,
-    'gitdir': do_gitdir,
 }
 
 
diff --git a/transport-helper.c b/transport-helper.c
index 660147f..3282693 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -105,6 +105,12 @@ static struct child_process *get_helper(struct transport *transport)
 	int refspec_alloc = 0;
 	int duped;
 	int code;
+	char git_dir_buf[sizeof(GIT_DIR_ENVIRONMENT) + PATH_MAX + 1];
+	const char *helper_env[] = {
+		git_dir_buf,
+		NULL
+	};
+
 
 	if (data->helper)
 		return data->helper;
@@ -120,6 +126,10 @@ static struct child_process *get_helper(struct transport *transport)
 	helper->argv[2] = remove_ext_force(transport->url);
 	helper->git_cmd = 0;
 	helper->silent_exec_failure = 1;
+
+	snprintf(git_dir_buf, sizeof(git_dir_buf), "%s=%s", GIT_DIR_ENVIRONMENT, get_git_dir());
+	helper->env = helper_env;
+
 	code = start_command(helper);
 	if (code < 0 && errno == ENOENT)
 		die("Unable to find remote helper for '%s'", data->name);
-- 
1.7.3.4
Dmitry Ivankov· Jul 13, 2011, 17:14 UTC · re: Dmitry Ivankov · lore

Re: [PATCH v2 3/3] remote-helpers: export GIT_DIR variable to helpers

+Sverre Sorry for the noise.

On Wed, Jul 13, 2011 at 11:10 PM, Dmitry Ivankov <divanorama@gmail.com> wrote:
Show 147 quoted lines
> The gitdir capability is recognized by git and can be used to tell
> the helper where the .git directory is. But it is not mentioned in
> the documentation and considered worse than if gitdir was passed
> via GIT_DIR environment variable.
>
> Do export GIT_DIR for a remote helper. Switch testgit to use env
> instead of less favoured capability gitdir.
>
> Mention it's possible uses in documentation: data storage, auxiliary
> git commands needed by a helper.
>
> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
> ---
> Instead of documenting capability gitdir export GIT_DIR. Turned out
> to be quite straightforward.
>
>  Documentation/git-remote-helpers.txt |   18 ++++++++++++++++++
>  git-remote-testgit.py                |   14 +-------------
>  transport-helper.c                   |   10 ++++++++++
>  3 files changed, 29 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
> index 58f6ad4..394fc8e 100644
> --- a/Documentation/git-remote-helpers.txt
> +++ b/Documentation/git-remote-helpers.txt
> @@ -47,6 +47,9 @@ arguments. The first argument specifies a remote repository as in git;
>  it is either the name of a configured remote or a URL. The second
>  argument specifies a URL; it is usually of the form
>  '<transport>://<address>', but any arbitrary string is possible.
> +'GIT_DIR' environment variable is set up for the remote helper and
> +can be used to store some additional data or to invoke auxiliary git
> +commands.
>
>  When git encounters a URL of the form '<transport>://<address>', where
>  '<transport>' is a protocol that it cannot handle natively, it
> @@ -159,6 +162,14 @@ Supported if the helper has the "import" capability.
>  +
>  Supported if the helper has the "connect" capability.
>
> +'gitdir' <path>::
> +       Tells helper the location of current repository .git
> +       directory. The path is absolute. The command is issued
> +       immediately after "gitdir" capability line is read by
> +       the caller.
> ++
> +Supported if the helper has the "gitdir" capability.
> +
>  If a fatal error occurs, the program writes the error message to
>  stderr and exits. The caller should expect that a suitable error
>  message has been printed if the child closes the connection without
> @@ -175,6 +186,7 @@ CAPABILITIES
>  'push'::
>  'import'::
>  'connect'::
> +'gitdir'::
>        This helper supports the corresponding command with the same name.
>
>  'refspec' 'spec'::
> @@ -187,6 +199,12 @@ CAPABILITIES
>        all, it must cover all refs reported by the list command; if
>        it is not used, it is effectively "{asterisk}:{asterisk}"
>
> +'gitdir'::
> +       The helper wants to know .git directory location. The gitdir
> +       command is sent immediately after reading this capability.
> +       The helper can read it immediately too or it can proceed with
> +       reporting other capabilities and read it as a next command.
> +
>  REF LIST ATTRIBUTES
>  -------------------
>
> diff --git a/git-remote-testgit.py b/git-remote-testgit.py
> index df9d512..df3e37f 100644
> --- a/git-remote-testgit.py
> +++ b/git-remote-testgit.py
> @@ -35,7 +35,7 @@ def get_repo(alias, url):
>     prefix = 'refs/testgit/%s/' % alias
>     debug("prefix: '%s'", prefix)
>
> -    repo.gitdir = ""
> +    repo.gitdir = os.environ["GIT_DIR"]
>     repo.alias = alias
>     repo.prefix = prefix
>
> @@ -70,7 +70,6 @@ def do_capabilities(repo, args):
>
>     print "import"
>     print "export"
> -    print "gitdir"
>     print "refspec refs/heads/*:%s*" % repo.prefix
>
>     print # end capabilities
> @@ -150,22 +149,11 @@ def do_export(repo, args):
>     repo.non_local.push(repo.gitdir)
>
>
> -def do_gitdir(repo, args):
> -    """Stores the location of the gitdir.
> -    """
> -
> -    if not args:
> -        die("gitdir needs an argument")
> -
> -    repo.gitdir = ' '.join(args)
> -
> -
>  COMMANDS = {
>     'capabilities': do_capabilities,
>     'list': do_list,
>     'import': do_import,
>     'export': do_export,
> -    'gitdir': do_gitdir,
>  }
>
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 660147f..3282693 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -105,6 +105,12 @@ static struct child_process *get_helper(struct transport *transport)
>        int refspec_alloc = 0;
>        int duped;
>        int code;
> +       char git_dir_buf[sizeof(GIT_DIR_ENVIRONMENT) + PATH_MAX + 1];
> +       const char *helper_env[] = {
> +               git_dir_buf,
> +               NULL
> +       };
> +
>
>        if (data->helper)
>                return data->helper;
> @@ -120,6 +126,10 @@ static struct child_process *get_helper(struct transport *transport)
>        helper->argv[2] = remove_ext_force(transport->url);
>        helper->git_cmd = 0;
>        helper->silent_exec_failure = 1;
> +
> +       snprintf(git_dir_buf, sizeof(git_dir_buf), "%s=%s", GIT_DIR_ENVIRONMENT, get_git_dir());
> +       helper->env = helper_env;
> +
>        code = start_command(helper);
>        if (code < 0 && errno == ENOENT)
>                die("Unable to find remote helper for '%s'", data->name);
> --
> 1.7.3.4
>
>
Sverre Rabbelier· Jul 13, 2011, 17:36 UTC · re: Dmitry Ivankov · lore

Re: [PATCH v2 3/3] remote-helpers: export GIT_DIR variable to helpers

Heya,
On Wed, Jul 13, 2011 at 19:10, Dmitry Ivankov <divanorama@gmail.com> wrote:
> Instead of documenting capability gitdir export GIT_DIR. Turned out
> to be quite straightforward.
But you _also_ documented the gitdir capability? I'm confused.
-- 
Cheers,

Sverre Rabbelier
Dmitry Ivankov· Jul 13, 2011, 18:10 UTC · re: Sverre Rabbelier · lore

Re: [PATCH v2 3/3] remote-helpers: export GIT_DIR variable to helpers

Hi!
On Wed, Jul 13, 2011 at 11:36 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
Show 7 quoted lines
> Heya,
>
> On Wed, Jul 13, 2011 at 19:10, Dmitry Ivankov <divanorama@gmail.com> wrote:
>> Instead of documenting capability gitdir export GIT_DIR. Turned out
>> to be quite straightforward.
>
> But you _also_ documented the gitdir capability? I'm confused.

Oww, it was a bad idea to amend previous commit in a hurry and not to read the whole diff after. Of course Documentation/git-remote-helpers.txt should have only a GIT_DIR hunk:

Show changes to Documentation/git-remote-helpers.txt +3 −0
diff --git a/Documentation/git-remote-helpers.txt
b/Documentation/git-remote-helpers.txt
index 58f6ad4..feda03e 100644
--- a/Documentation/git-remote-helpers.txt
+++ b/Documentation/git-remote-helpers.txt
@@ -47,6 +47,9 @@ arguments. The first argument specifies a remote
repository as in git;
 it is either the name of a configured remote or a URL. The second
 argument specifies a URL; it is usually of the form
 '<transport>://<address>', but any arbitrary string is possible.
+'GIT_DIR' environment variable is set up for the remote helper and
+can be used to store some additional data or to invoke auxiliary git
+commands.

 When git encounters a URL of the form '<transport>://<address>', where
 '<transport>' is a protocol that it cannot handle natively, it



>
> --
> Cheers,
>
> Sverre Rabbelier
>

← back to recent threads