Git development
 help / color / mirror / Atom feed
* extended hook api and tweak-fetch hook
From: Joey Hess @ 2011-12-30  1:07 UTC (permalink / raw)
  To: git

This patch series adds an extended hook API, and uses it to implement
the new tweak-fetch hook.

The remaining hooks (that do not already use run_hook()) could be
refactored later to use this new API.

Also, the API has been designed to allow several programs to be run
for a single hook, when someone wants to add that into git.

^ permalink raw reply

* [PATCH 1/3] expanded hook api with stdio support
From: Joey Hess @ 2011-12-30  1:07 UTC (permalink / raw)
  To: git; +Cc: Joey Hess
In-Reply-To: <1325207240-22622-1-git-send-email-joey@kitenet.net>

Adds run_hook_complex() and the struct hook, and implements
run_hook() using it.

This new API for hooks will allow for a later refactoring of the existing
ad-hoc hook code for hooks that are fed input on stdin, such as pre-receive,
post-receive, and post-rewrite.

It also adds support for a new class of hooks whose stdout is processed by
git, as well as hooks that both receive stdin and have their stdout
processed. This is controlled by setting "generator" and "reader" members
of the struct hook.

The API provides control over whether a hook must consume all its stdin,
or is free to ignore some of it; this can be specified by using either
feed_hook_in_full() or feed_hook_incomplete() as the "feeder" member of
the struct hook. The stdin feeder runs asynchronously, to avoid blocking
when the hook's stdin is also being read. So the API design limits the
code that needs to run asynchronously, to make it easy to implement
thread-safe feeders.

Finally, the API is designed to be extended in the future, to support
running multiple programs for a single hook action (such as the contents
of a .git/hooks/hook.d/ , or a system-wide hook). This design goal led
to the "generator" and "reader" members of the struct hook, which are
specified such that they can be called repeatedly, with data flowing
between them (via the "data" member), like this:
    generator | hook_prog_1 | reader | generator | hook_prog_2 | reader

Signed-off-by: Joey Hess <joey@kitenet.net>
---
 Documentation/technical/api-run-command.txt |   53 +++++++++++
 run-command.c                               |  132 ++++++++++++++++++++++++---
 run-command.h                               |   58 +++++++++++-
 3 files changed, 226 insertions(+), 17 deletions(-)

diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt
index f18b4f4..ff50d2f 100644
--- a/Documentation/technical/api-run-command.txt
+++ b/Documentation/technical/api-run-command.txt
@@ -87,6 +87,17 @@ The functions above do the following:
 	On execution, .stdout_to_stderr and .no_stdin will be set.
 	(See below.)
 
+`run_hook_complex`::
+
+	Run a hook, with the caller providing its stdin and/or parsing its
+	stdout.
+	Takes a pointer to a `struct hook` that specifies the details,
+	including the name of the hook, any parameters to pass to it,
+	and how to handle the stdin and stdout. (See below.)
+	If the hook does not exist or is not executable, the return value
+	will be zero.
+	If it is executable, the hook will be executed and the exit
+	status of the hook is returned.
 
 Data structures
 ---------------
@@ -241,3 +252,45 @@ a forked process otherwise:
 
 . It must not change the program's state that the caller of the
   facility also uses.
+
+* `struct hook`
+
+This describes a hook to run.
+
+The caller:
+
+1. allocates and clears (memset(&hook, 0, sizeof(hook));) a
+   struct hook variable;
+2. initializes the members;
+3. calls hook();
+4. if necessary, accesses data read from the hook in .data
+5. frees the struct hook.
+
+The `struct hook` has three function pointers that may be set to
+control the stdin that is sent to the hook, and to collect its stdout.
+
+The `generator` generates the stdin for the hook, returning it in a strbuf.
+It is passed a pointer to the `struct hook`.
+
+The `feeder` is run asynchronously to feed the generated stdin into the hook.
+It is passed the handle to write to, the strbuf containing the stdin, and 
+a pointer to the `struct hook`, and should return non-zero on failure.
+Typically it is set to either `feed_hook_in_full`, or `feed_hook_incomplete`.
+
+The `reader` reads and processes the hook's stdout. It is passed 
+a handle to read from and a pointer to the `struct hook`, and should return
+non-zero on failure.
+
+If the generator or feeder are NULL, the hook is not fed anything
+on stdin; if the `reader` is NULL, the hook's stdout is sent to
+stderr instead.
+
+Note that in the future, the generator, feeder, and reader might be run
+more than once, if multiple programs are run as part of a single hook.
+Therefore, all three should avoid taking any actions except for generating
+the stdin, writing it to the hook, reading/parsing the hook's stdout,
+and printing any necessary warning messages.
+
+The `struct hook` also contains a `data` pointer, which can be used
+to communicate between the generator, feeder, reader, and the
+caller of the hook.
diff --git a/run-command.c b/run-command.c
index 1c51043..42a9b06 100644
--- a/run-command.c
+++ b/run-command.c
@@ -605,34 +605,136 @@ int finish_async(struct async *async)
 
 int run_hook(const char *index_file, const char *name, ...)
 {
-	struct child_process hook;
+	struct hook hook;
 	struct argv_array argv = ARGV_ARRAY_INIT;
-	const char *p, *env[2];
-	char index[PATH_MAX];
+	const char *p;
 	va_list args;
 	int ret;
 
-	if (access(git_path("hooks/%s", name), X_OK) < 0)
-		return 0;
-
 	va_start(args, name);
-	argv_array_push(&argv, git_path("hooks/%s", name));
 	while ((p = va_arg(args, const char *)))
 		argv_array_push(&argv, p);
 	va_end(args);
 
 	memset(&hook, 0, sizeof(hook));
-	hook.argv = argv.argv;
-	hook.no_stdin = 1;
-	hook.stdout_to_stderr = 1;
-	if (index_file) {
-		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+	hook.name = name;
+	hook.index_file = index_file;
+	hook.argv_array = &argv;
+	ret = run_hook_complex(&hook);
+
+	argv_array_clear(&argv);
+	return ret;
+}
+
+struct feed_hook_with {
+	struct strbuf *buf;
+	struct hook *hook;
+};
+
+/*
+ * An async process is used to feed the hook its stdin.
+ * This allows the hook to read and write at its own pace without blocking.
+ */
+static int feed_hook_async(int in, int out, void *data)
+{
+	struct feed_hook_with *feedwith = data;
+	int ret = feedwith->hook->feeder(out, feedwith->buf, feedwith->hook);
+	close(out);
+	return ret;
+}
+
+/*
+ * Runs a hook, if it exists. Returns non-zero if the hook fails to run
+ * correctly.
+ */
+int run_hook_complex(struct hook *hook)
+{
+	char *command;
+	struct child_process child;
+	struct async async;
+	struct feed_hook_with feedwith = { 0, hook };
+	struct argv_array argv = ARGV_ARRAY_INIT;
+	const char *env[2];
+	char index[PATH_MAX];
+	int res = 0;
+	int i;
+
+	command = git_path("hooks/%s", hook->name);
+	if (access(command, X_OK) < 0)
+		return 0;
+
+	memset(&child, 0, sizeof(child));
+	argv_array_push(&argv, command);
+	if (hook->argv_array)
+		for (i = 0; i < hook->argv_array->argc; i++)
+			argv_array_push(&argv, hook->argv_array->argv[i]);
+	child.argv = argv.argv;
+	if (hook->generator && hook->feeder)
+		child.in = -1;
+	else
+		child.no_stdin = 1;
+	if (hook->reader)
+		child.out = -1;
+	else
+		child.stdout_to_stderr = 1;
+	if (hook->index_file) {
+		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s",
+				hook->index_file);
 		env[0] = index;
 		env[1] = NULL;
-		hook.env = env;
+		child.env = env;
+	}
+	res |= start_command(&child);
+	if (res) goto hook_out;
+
+	if (hook->generator && hook->feeder) {
+		feedwith.buf = hook->generator(hook);
+		if (! feedwith.buf) {
+			res = 1;
+			goto hook_out;
+		}
+
+		memset(&async, 0, sizeof(async));
+		async.proc = feed_hook_async;
+		async.data = &feedwith;
+		async.out = child.in;
+		res |= start_async(&async);
+		if (res) {
+			close(child.in);
+			close(child.out);
+			finish_command(&child);
+			goto hook_out;
+		}
 	}
 
-	ret = run_command(&hook);
+	if (hook->reader)
+		res |= hook->reader(child.out, hook);
+	if (hook->generator && hook->feeder)
+	       res |= finish_async(&async);
+	res |= finish_command(&child);
+
+ hook_out:
+	if (feedwith.buf)
+		strbuf_release(feedwith.buf);
 	argv_array_clear(&argv);
-	return ret;
+
+	return res;
+}
+
+/* A feeder that ensures the hook consumes all its stdin. */
+int feed_hook_in_full(int handle, struct strbuf *buf, struct hook *hook)
+{
+	if (write_in_full(handle, buf->buf, buf->len) != buf->len) {
+		warning("%s hook failed to consume all its input", hook->name);
+		return 1;
+	}
+	else
+		return 0;
+}
+
+/* A feeder that does not require the hook consume all its stdin. */
+int feed_hook_incomplete(int handle, struct strbuf *buf, struct hook *hook)
+{
+	write_in_full(handle, buf->buf, buf->len);
+	return 0;
 }
diff --git a/run-command.h b/run-command.h
index 56491b9..54c9b83 100644
--- a/run-command.h
+++ b/run-command.h
@@ -45,8 +45,6 @@ int start_command(struct child_process *);
 int finish_command(struct child_process *);
 int run_command(struct child_process *);
 
-extern int run_hook(const char *index_file, const char *name, ...);
-
 #define RUN_COMMAND_NO_STDIN 1
 #define RUN_GIT_CMD	     2	/*If this is to be git sub-command */
 #define RUN_COMMAND_STDOUT_TO_STDERR 4
@@ -90,4 +88,60 @@ struct async {
 int start_async(struct async *async);
 int finish_async(struct async *async);
 
+/*
+ * This data structure controls how a hook is run.
+ */
+struct hook {
+	/* The name of the hook being run. */
+	const char *name;
+	/*
+	 * Parameters to pass to the hook program, not including the name
+	 * of the hook. May be NULL.
+	 */
+	struct argv_array *argv_array;
+	/*
+	 * Pathname to an index file to use, or NULL if the hook
+	 * uses the default index file or no index is needed.
+	 */
+	const char *index_file;
+	/*
+	 * An arbitrary data structure, can be used to communicate between
+	 * the generator, feeder, reader, and the caller of the hook.
+	 */
+	void *data;
+	/*
+	 * Populates a strbuf with the content to send to the
+	 * hook on its standard input.
+	 *
+	 * May be NULL, if the hook does not consume standard input.
+	 */
+	struct strbuf *(*generator)(struct hook *hook);
+	/*
+	 * Feeds generated content to the hook on its standard input
+	 * via the handle. Returns non-zero on failure.
+	 *
+	 * May be NULL, if the hook does not consume standard input.
+	 *
+	 * Note that the feeder is run as an async process, and so should
+	 * avoid modifying any global state, and only use thread-safe
+	 * operations. It may be run more than once.
+	 */
+	int (*feeder)(int handle, struct strbuf *data, struct hook *hook);
+	/*
+	 * Processes the hook's standard output from the handle,
+	 * returning non-zero on failure.
+	 *
+	 * May be NULL, if the hook's stdin is not processed. (It will
+	 * instead be redirected to stderr.)
+	 */
+	int (*reader)(int handle, struct hook *hook);
+};
+
+extern int run_hook(const char *index_file, const char *name, ...);
+
+extern int run_hook_complex(struct hook *hook);
+
+extern int feed_hook_in_full(int handle, struct strbuf *buf, struct hook *hook);
+extern int feed_hook_incomplete(int handle, struct strbuf *buf, struct hook *hook);
+
 #endif
-- 
1.7.7.3

^ permalink raw reply related

* [PATCH 3/3] add tweak-fetch hook
From: Joey Hess @ 2011-12-30  1:07 UTC (permalink / raw)
  To: git; +Cc: Joey Hess
In-Reply-To: <1325207240-22622-1-git-send-email-joey@kitenet.net>

The tweak-fetch hook is fed lines on stdin for all refs that were fetched,
and outputs on stdout possibly modified lines. Its output is parsed and
used when git fetch updates the remote tracking refs, records the entries
in FETCH_HEAD, and produces its report.

This is implemented using the new run_hook_complex API.

Signed-off-by: Joey Hess <joey@kitenet.net>
---
 Documentation/githooks.txt |   29 +++++++++
 builtin/fetch.c            |  144 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 172 insertions(+), 1 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 28edefa..bea450a 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -162,6 +162,35 @@ This hook can be used to perform repository validity checks, auto-display
 differences from the previous HEAD if different, or set working dir metadata
 properties.
 
+tweak-fetch
+~~~~~~~~~~~
+
+This hook is invoked by 'git fetch' (commonly called by 'git pull'), after
+refs have been fetched from the remote repository. It is not executed, if
+nothing was fetched.
+
+The output of the hook is used to update the remote-tracking branches, and
+`.git/FETCH_HEAD`, in preparation for for a later merge operation done by
+'git merge'.
+
+It takes no arguments, but is fed a line of the following format on
+its standard input for each ref that was fetched.
+
+  <sha1> SP not-for-merge|merge SP <remote-refname> SP <local-refname> LF
+
+Where the "not-for-merge" flag indicates the ref is not to be merged into the
+current branch, and the "merge" flag indicates that 'git merge' should
+later merge it. The `<remote-refname>` is the remote's name for the ref
+that was fetched, and `<local-refname>` is a name of a remote-tracking branch,
+like "refs/remotes/origin/master", or can be empty if the fetched ref is not
+being stored in a local refname.
+
+The hook must consume all of its standard input, and output back lines
+of the same format. It can modify its input as desired, including
+adding or removing lines, updating the sha1 (i.e. re-point the
+remote-tracking branch), changing the merge flag, and changing the
+`<local-refname>` (i.e. use different remote-tracking branch).
+
 post-merge
 ~~~~~~~~~~
 
diff --git a/builtin/fetch.c b/builtin/fetch.c
index a48358a..80178d0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -103,6 +103,148 @@ static int add_existing(const char *refname, const unsigned char *sha1,
 	return 0;
 }
 
+struct strbuf *tweak_fetch_hook_generator(struct hook *hook)
+{
+	struct ref *ref;
+	struct strbuf *buf = xmalloc(sizeof(struct strbuf));
+
+	strbuf_init(buf, 128);
+	for (ref = hook->data; ref; ref = ref->next)
+		strbuf_addf(buf, "%s %s %s %s\n",
+			sha1_to_hex(ref->old_sha1),
+			ref->merge ? "merge" : "not-for-merge",
+			ref->name ? ref->name : "",
+			ref->peer_ref && ref->peer_ref->name ?
+				ref->peer_ref->name : "");
+	return buf;
+}
+
+static struct ref *parse_tweak_fetch_hook_line(char *l, 
+		struct string_list *existing_refs, struct hook *hook)
+{
+	struct ref *ref = NULL, *peer_ref = NULL;
+	struct string_list_item *peer_item = NULL;
+	char *p, *words[4];
+	char *problem;
+	int i;
+
+	for (i = 0 ; i <= ARRAY_SIZE(words) - 1; i++) {
+		p = strchr(l, ' ');
+		words[i] = l;
+		if (!p)
+			break;
+		p[0] = '\0';
+		l = p+1;
+	}
+	if (i != ARRAY_SIZE(words) - 1) {
+		problem = "wrong number of words";
+		goto unparsable;
+	}
+
+	ref = alloc_ref(words[2]);
+	peer_ref = ref->peer_ref = alloc_ref(words[3]);
+	ref->peer_ref->force = 1;
+
+	if (get_sha1_hex(words[0], ref->old_sha1)) {
+		problem = "bad sha1";
+		goto unparsable;
+	}
+
+	if (!strcmp(words[1], "merge")) {
+		ref->merge = 1;
+	}
+	else if (strcmp(words[1], "not-for-merge")) {
+		problem = "bad merge flag";
+		goto unparsable;
+	}
+
+	peer_item = string_list_lookup(existing_refs, peer_ref->name);
+	if (peer_item)
+		hashcpy(peer_ref->old_sha1, peer_item->util);
+
+	return ref;
+
+ unparsable:
+	warning("%s hook output a wrongly formed line: %s",
+			hook->name, problem);
+	free(ref);
+	free(peer_ref);
+	return NULL;
+}
+
+int tweak_fetch_hook_reader(int handle, struct hook *hook)
+{
+	FILE *f;
+	struct strbuf buf;
+	struct string_list existing_refs = STRING_LIST_INIT_NODUP;
+	struct ref *old_refs, *ref, *prevref = NULL;
+	int res = 0;
+
+	f = fdopen(handle, "r");
+	if (f == NULL)
+		return 1;
+
+	old_refs = hook->data;
+	hook->data = NULL;
+
+	strbuf_init(&buf, 128);
+	for_each_ref(add_existing, &existing_refs);
+
+	while (strbuf_getline(&buf, f, '\n') != EOF) {
+		char *l = strbuf_detach(&buf, NULL);
+		ref = parse_tweak_fetch_hook_line(l, &existing_refs, hook);
+		if (ref) {
+			if (prevref) {
+				prevref->next = ref;
+				prevref = ref;
+			} else {
+				hook->data = prevref = ref;
+			}
+		} else {
+			res = 1;
+		}
+		free(l);
+	}
+
+	string_list_clear(&existing_refs, 0);
+	strbuf_release(&buf);
+	fclose(f);
+
+	if (res)
+		hook->data = old_refs;
+	else
+		free_refs(old_refs);
+
+	return res;
+}
+
+/*
+ * The tweak-fetch hook is fed lines of the form:
+ * <sha1> SP <not-for-merge|merge> SP <remote-refname> SP <local-refname> LF
+ * And should output rewritten lines of the same form, which are used
+ * to generate a tweaked set of fetched_refs.
+ */
+static struct ref *run_tweak_fetch_hook(struct ref *fetched_refs)
+{
+	struct hook hook;
+
+	if (! fetched_refs)
+		return fetched_refs;
+
+	memset(&hook, 0, sizeof(hook));
+	hook.name = "tweak-fetch";
+	hook.generator = tweak_fetch_hook_generator;
+	hook.feeder = feed_hook_in_full;
+	hook.reader = tweak_fetch_hook_reader;
+	hook.data = fetched_refs;
+
+	if (run_hook_complex(&hook)) {
+		warning("%s hook failed, ignoring its output", hook.name);
+	}
+
+	return hook.data;
+}
+
 static void unlock_pack(void)
 {
 	if (transport)
@@ -529,7 +671,7 @@ static struct refs_result fetch_refs(struct transport *transport,
 	if (res.status)
 		res.status = transport_fetch_refs(transport, ref_map);
 	if (!res.status) {
-		res.new_refs = ref_map;
+		res.new_refs = run_tweak_fetch_hook(ref_map);
 
 		res.status |= store_updated_refs(transport->url,
 				transport->remote->name,
-- 
1.7.7.3

^ permalink raw reply related

* [PATCH 2/3] preparations for tweak-fetch hook
From: Joey Hess @ 2011-12-30  1:07 UTC (permalink / raw)
  To: git; +Cc: Joey Hess
In-Reply-To: <1325207240-22622-1-git-send-email-joey@kitenet.net>

No behavior changes yet, only some groundwork for the next
change.

The refs_result structure combines a status code with a ref map,
which can be NULL even on success. This will be needed when
there's a tweak-fetch hook, because it can filter out all refs,
while still succeeding.

fetch_refs returns a refs_result, so that it can modify the ref_map.

Signed-off-by: Joey Hess <joey@kitenet.net>
---
 builtin/fetch.c |   55 ++++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 33ad3aa..a48358a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -29,6 +29,11 @@ enum {
 	TAGS_SET = 2
 };
 
+struct refs_result {
+	struct ref *new_refs;
+	int status;
+};
+
 static int all, append, dry_run, force, keep, multiple, prune, update_head_ok, verbosity;
 static int progress, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int tags = TAGS_DEFAULT;
@@ -89,6 +94,15 @@ static struct option builtin_fetch_options[] = {
 	OPT_END()
 };
 
+static int add_existing(const char *refname, const unsigned char *sha1,
+			int flag, void *cbdata)
+{
+	struct string_list *list = (struct string_list *)cbdata;
+	struct string_list_item *item = string_list_insert(list, refname);
+	item->util = (void *)sha1;
+	return 0;
+}
+
 static void unlock_pack(void)
 {
 	if (transport)
@@ -507,17 +521,25 @@ static int quickfetch(struct ref *ref_map)
 	return check_everything_connected(iterate_ref_map, 1, &rm);
 }
 
-static int fetch_refs(struct transport *transport, struct ref *ref_map)
+static struct refs_result fetch_refs(struct transport *transport,
+		struct ref *ref_map)
 {
-	int ret = quickfetch(ref_map);
-	if (ret)
-		ret = transport_fetch_refs(transport, ref_map);
-	if (!ret)
-		ret |= store_updated_refs(transport->url,
+	struct refs_result res;
+	res.status = quickfetch(ref_map);
+	if (res.status)
+		res.status = transport_fetch_refs(transport, ref_map);
+	if (!res.status) {
+		res.new_refs = ref_map;
+
+		res.status |= store_updated_refs(transport->url,
 				transport->remote->name,
-				ref_map);
+				res.new_refs);
+	}
+	else {
+		res.new_refs = ref_map;
+	}
 	transport_unlock_pack(transport);
-	return ret;
+	return res;
 }
 
 static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)
@@ -542,15 +564,6 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)
 	return result;
 }
 
-static int add_existing(const char *refname, const unsigned char *sha1,
-			int flag, void *cbdata)
-{
-	struct string_list *list = (struct string_list *)cbdata;
-	struct string_list_item *item = string_list_insert(list, refname);
-	item->util = (void *)sha1;
-	return 0;
-}
-
 static int will_fetch(struct ref **head, const unsigned char *sha1)
 {
 	struct ref *rm = *head;
@@ -673,6 +686,7 @@ static int do_fetch(struct transport *transport,
 	struct string_list_item *peer_item = NULL;
 	struct ref *ref_map;
 	struct ref *rm;
+	struct refs_result res;
 	int autotags = (transport->remote->fetch_tags == 1);
 
 	for_each_ref(add_existing, &existing_refs);
@@ -710,7 +724,9 @@ static int do_fetch(struct transport *transport,
 
 	if (tags == TAGS_DEFAULT && autotags)
 		transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
-	if (fetch_refs(transport, ref_map)) {
+	res = fetch_refs(transport, ref_map);
+	ref_map = res.new_refs;
+	if (res.status) {
 		free_refs(ref_map);
 		return 1;
 	}
@@ -750,7 +766,8 @@ static int do_fetch(struct transport *transport,
 		if (ref_map) {
 			transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
 			transport_set_option(transport, TRANS_OPT_DEPTH, "0");
-			fetch_refs(transport, ref_map);
+			res = fetch_refs(transport, ref_map);
+			ref_map = res.new_refs;
 		}
 		free_refs(ref_map);
 	}
-- 
1.7.7.3

^ permalink raw reply related

* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users
From: Sven Strickroth @ 2011-12-30  4:40 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jakub Narebski, Jeff King
In-Reply-To: <7vlipwz5xs.fsf@alter.siamese.dyndns.org>

Am 28.12.2011 23:29 schrieb Junio C Hamano:
>>> I suspect that we would need to enhance *_ASKPASS interface first, so that
>>> we can ask things other than passwords. Until that happens, I do not think
>>> we should apply the second patch to use *_ASKPASS for non-passwords.
>>
>> git-core also asks for username using *_ASKPASS, this is the reason why
>> I implemented it this way. I noticed it when I tried to push to google
>> code (using https).
> 
> I thought that was updated with Peff's series recently?

So, was this changed? git-core doesn't ask for a username using
*_ASKPASS helpers anymore?

> In any case, your username has a lot minor annoyance factor if we force
> you to type in blind, but the second patch in your series ask things other
> than that using the same mechanism, so it is not a good excuse for this
> usability regression in git-svn, I would think.

Yeah, typing a whole path is more annoying. But not being able to clone
(push, ...) w/o being able to type in a username or accept an unknown
certificate is also problematic.

I talked off-list with Junio and he proposed to use another environment
variable (e.g. GIT_DIALOG for a different tool) to solve these issues.

A good way could be to define the GIT_DIALOG-tools to have two
parameters. First (pass|text|filename|...) with fallback to text, this
way one can implement a password field, a text field, a file chooser (on
type filename) and it is still extendable for e.g. directory choosers
(if we might need that)...

To make it even more complicated one could also say that we propose more
parameters (e.g. two additional parameters for working copy directory
and repository), so that answers can be stored by the dialog-helper in
the .git-directory.

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply

* Re: [PATCH 1/3] expanded hook api with stdio support
From: Johannes Sixt @ 2011-12-30  9:47 UTC (permalink / raw)
  To: Joey Hess; +Cc: git
In-Reply-To: <1325207240-22622-2-git-send-email-joey@kitenet.net>

Am 30.12.2011 02:07, schrieb Joey Hess:
> Finally, the API is designed to be extended in the future, to support
> running multiple programs for a single hook action (such as the contents
> of a .git/hooks/hook.d/ , or a system-wide hook). This design goal led
> to the "generator" and "reader" members of the struct hook, which are
> specified such that they can be called repeatedly, with data flowing
> between them (via the "data" member), like this:
>     generator | hook_prog_1 | reader | generator | hook_prog_2 | reader

IMHO, this is overengineered. I don't think that we need something like
this in the foreseeable future, particularly because such a pipeline or
multi-hook infrastructure can easily be constructed by the (single) hook
script itself.

IOW, none of the three function pointers should be needed (not even the
feeder, see below), at least not in a first step.

IMO, as the first step, the user of this infrastructure should only be
required to construct the hook input as a strbuf, and receive the hook
output, if needed, also as a strbuf.

> +`run_hook_complex`::
> +
> +	Run a hook, with the caller providing its stdin and/or parsing its
> +	stdout.
> +	Takes a pointer to a `struct hook` that specifies the details,
> +	including the name of the hook, any parameters to pass to it,
> +	and how to handle the stdin and stdout. (See below.)
> +	If the hook does not exist or is not executable, the return value
> +	will be zero.
> +	If it is executable, the hook will be executed and the exit
> +	status of the hook is returned.

What is the rationale for these error modes? It is as if a non-existent
or non-executable hook counts as 'success'. (I'm not saying that this
would be wrong, I'm just asking.)

>  
>  Data structures
>  ---------------
> @@ -241,3 +252,45 @@ a forked process otherwise:
>  
>  . It must not change the program's state that the caller of the
>    facility also uses.
> +
> +* `struct hook`
> +
> +This describes a hook to run.
> +
> +The caller:
> +
> +1. allocates and clears (memset(&hook, 0, sizeof(hook));) a
> +   struct hook variable;
> +2. initializes the members;
> +3. calls hook();

run_hook_complex()?

> +4. if necessary, accesses data read from the hook in .data
> +5. frees the struct hook.

> +The `feeder` is run asynchronously to feed the generated stdin into the hook.
> +It is passed the handle to write to, the strbuf containing the stdin, and 
> +a pointer to the `struct hook`, and should return non-zero on failure.
> +Typically it is set to either `feed_hook_in_full`, or `feed_hook_incomplete`.

IMO, this is overengineered. See below.

> +	res |= start_command(&child);
> +	if (res) goto hook_out;

Please write this conditional in two lines.

> +/* A feeder that ensures the hook consumes all its stdin. */
> +int feed_hook_in_full(int handle, struct strbuf *buf, struct hook *hook)
> +{
> +	if (write_in_full(handle, buf->buf, buf->len) != buf->len) {
> +		warning("%s hook failed to consume all its input", hook->name);

Really? Could there not be any other error condition? The warning would
be correct only if errno == EPIPE, and this error will be returned only
if SIGPIPE is ignored. Does this happen anywhere?

Futhermore, if all data was written to the pipe successfully, there is
no way to know whether the reader consumed everything.

IOW, I don't it is worth to make the distinction. However, I do think
that the parent process must be protected against SIGPIPE.

> +		return 1;
> +	}
> +	else
> +		return 0;
> +}

-- Hannes

^ permalink raw reply

* Re: [PATCH] stash: Don't fail if work dir contains file named 'HEAD'
From: Thomas Rast @ 2011-12-30 10:15 UTC (permalink / raw)
  To: Jonathon Mah; +Cc: git, Junio C Hamano
In-Reply-To: <913BB2F9-3C51-44D0-BFEC-3A49A5EC9E15@JonathonMah.com>

Jonathon Mah <me@JonathonMah.com> writes:
> When performing a plain "git stash" (without --patch), git-diff would fail
> with "fatal: ambiguous argument 'HEAD': both revision and filename". The
> output was piped into git-update-index, masking the failed exit status.
> The output is now sent to a temporary file (which is cleaned up by
> existing code), and the exit status is checked. The "HEAD" arg to the
> git-diff invocation has been disambiguated too, of course.

Thanks, good catch.

> In patch mode, "git stash -p" would fail harmlessly, leaving the working
> dir untouched.

Note that this only affects stash -p, not add/reset/commit -p, because
it is the only one that does an extra patch dance on top of the
git-add--interactive work.  stash -p uses a 'diff-index -p HEAD'
invocation in the %patch_modes of git-add--interactive, but diff-index
doesn't need disambiguation as the first argument is always the (sole)
tree-ish.

I had to look and verify, so perhaps you can put a paragraph to this
effect in the commit message.

> -			git diff --name-only -z HEAD | git update-index -z --add --remove --stdin &&
> +			git diff --name-only -z HEAD -- > "$TMP-stagenames" &&
> +			git update-index -z --add --remove --stdin < "$TMP-stagenames" &&

Style nit: we usually spell it >foo.  I saw that git-stash is already
inconsistent, but let's at least not make it worse.

While reading this I also wondered if there was a good reason it didn't
just use 'add -u', and indeed: 7aa5d43 (stash: Don't overwrite files
that have gone from the index, 2010-04-18) changed it *away* from add -u
because that was broken.

> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
[...]
> +test_expect_success 'stash where working directory contains "HEAD" file' '
> +	git stash clear &&
> +	git reset --hard &&
> +	echo file-not-a-ref > HEAD &&
> +	git add HEAD &&
> +	git stash &&
> +	git diff-files --quiet &&
> +	git diff-index --cached --quiet HEAD &&
> +	test_tick &&

What's the tick good for if you don't create any commits after it?  You
should put it immediately before the 'git stash'.

> +	test $(git rev-parse stash^) = $(git rev-parse HEAD) &&

This should probably have its arguments quoted, to avoid confusing
'test' if something goes horribly wrong (e.g. stash^ is not valid).
There are plenty of existing lines of this form in other tests however.

> +	git diff stash^..stash > output &&
> +	test_cmp output expect &&
> +	git stash drop

Perhaps this could go after the 'git stash' as a

  test_when_finished "git stash drop"

> +'

> diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
[...]
> -# note: bar sorts before dir, so the first 'n' is always to skip 'bar'
> +# note: order of files with unstaged changes: HEAD bar dir/foo
>  
>  test_expect_success PERL 'saying "n" does nothing' '
> +	set_state HEAD HEADfile_work HEADfile_index &&
>  	set_state dir/foo work index &&
> -	(echo n; echo n) | test_must_fail git stash save -p &&
> -	verify_state dir/foo work index &&
> -	verify_saved_state bar
> +	(echo n; echo n; echo n) | test_must_fail git stash save -p &&
> +	verify_state HEAD HEADfile_work HEADfile_index &&
> +	verify_saved_state bar &&
> +	verify_state dir/foo work index
>  '

Other reviewers may want to read these hunks in word diff mode, where it
is far easier to verify that the functionality tested is a superset:

  test_expect_success PERL 'saying "n" does nothing' '
          {+set_state HEAD HEADfile_work HEADfile_index &&+}
          set_state dir/foo work index &&
          (echo n; echo {+n; echo+} n) | test_must_fail git stash save -p &&
          verify_state [-dir/foo work index-]{+HEAD HEADfile_work HEADfile_index+} &&
          verify_saved_state bar {+&&+}
  {+      verify_state dir/foo work index+}
  '

> diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
[...]
>  test_expect_success 'stash save --include-untracked stashed the untracked files' '
>  	test "!" -f file2 &&
>  	test ! -e untracked &&
> -	git diff HEAD stash^3 -- file2 untracked >actual &&
> +	test "!" -f HEAD &&
> +	git diff HEAD stash^3 -- HEAD file2 untracked >actual &&

Again not something you started, but we may want to clean this up to say

  test_path_is_missing file2 &&
  test_path_is_missing untracked &&
  test_path_is_missing HEAD &&

etc.  This is nicer (gives a message) if the test fails.  It also barfs
if there is a freak bug that made HEAD a device or some such :-)


Anyway, except for the test_tick those were just style nits.  You can
add

  Acked-by: Thomas Rast <trast@student.ethz.ch>

when you reroll.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

^ permalink raw reply

* Git clone - head not pointing to master
From: Vibin Nair @ 2011-12-30 11:22 UTC (permalink / raw)
  To: git

Hi,
  I am a Git beginner. Recently i started working on a codebase (that is 
checked in to a git repository) of our client, who is not very aware 
about how git works.
The repository has multiple branches such as

origin/user1
origin/master
origin/user2

Now when i take a clone of this repository : git clone <repo-url>, a 
clone of the repo is created on my machine.

But when i do a : "git branch" it shows

user1 *

which means that the "head" is pointing to the "user1" branch.

The client told me that all the recent code were being merged into the 
"master" branch and hence the "user1" branch has code which is older 
than "master" branch.

I had 2 queries :

1. Is it possible to make the "head" of the remote point to "master" 
instead of "user1", so that other members of my team get the latest code 
when they try to clone the repository.
2. Is there any way to safely merge "user1" to "master".

^ permalink raw reply

* What's the best way to push/fetch replace refs?
From: Slawomir Testowy @ 2011-12-30 11:59 UTC (permalink / raw)
  To: git

I want to use git-replace to connect two disjoined commits (git-svn didn't
detect massive file movement) and I want to share these replace refs with others
(we're moving from svn to git). I cannot use git-rebase because the history
after the split contains lots of branches and merges and using git-rebase for
that is an overkill.

I did something like this:

# Setup some repositories and create basic commit.
git init --bare repo
git init a
git init b
cd a
echo a > a && git add a && git commit -m initial
git branch tmp
echo b > b && git add b && git commit -m second
git checkout tmp
echo c > c && git add c && git commit -m replacement

# Replace a commit with another one
git replace `git rev-parse master` `git rev-parse tmp`

# Push changes to public repo; this doesn't push replace refs.
git push --all ../repo/

# Is this the best way to push replace refs?
git push  ../repo/ 'refs/replace/*'

# Fetch commits from public repo
cd ../b/
git pull ../repo/

# We didn't fetch replace refs in previous git-pull. But this seems to
work. Is this the correct way?
git fetch ../repo/ 'refs/replace/*:refs/replace/*'

# I also added "fetch = +refs/replace/*:refs/replace/*" to [remote
"origin"] in "b" and this also seems to work.

^ permalink raw reply

* Re: Git clone - head not pointing to master
From: Konstantin Khomoutov @ 2011-12-30 12:40 UTC (permalink / raw)
  To: Vibin Nair; +Cc: git
In-Reply-To: <4EFD9F11.5030309@volitionlabs.com>

On Fri, 30 Dec 2011 16:52:57 +0530
Vibin Nair <vibin@volitionlabs.com> wrote:

>   I am a Git beginner. Recently i started working on a codebase (that
> is checked in to a git repository) of our client, who is not very
> aware about how git works.
> The repository has multiple branches such as
> 
> origin/user1
> origin/master
> origin/user2
> 
> Now when i take a clone of this repository : git clone <repo-url>, a 
> clone of the repo is created on my machine.
> 
> But when i do a : "git branch" it shows
> 
> user1 *
> 
> which means that the "head" is pointing to the "user1" branch.
> 
> The client told me that all the recent code were being merged into
> the "master" branch and hence the "user1" branch has code which is
> older than "master" branch.
> 
> I had 2 queries :
> 
> 1. Is it possible to make the "head" of the remote point to "master" 
> instead of "user1", so that other members of my team get the latest
> code when they try to clone the repository.
`git clone` checks out the branch which the HEAD ref in the remote repo
points to.  You can use `git symbolic-ref` to retarget HEAD if it's a
bare repository.  If the repository is non-bare HEAD tracks what's
checked out, so it can be changed by doing `git checkout`.
Note that you can pass the "-b <branch>" command-line option to `git
clone` to make it create a local tracking branch and check it out for
an arbitrary remote branch <branch>.

> 2. Is there any way to safely merge "user1" to "master".
This question has little sense because it heavily depends on what you
mean by "safely".  The correct way to state a question like this is to
think whether merging user1 to master has sense from the project's
development standpoint.

^ permalink raw reply

* Git -  server mediator
From: gruziak89 @ 2011-12-30 12:43 UTC (permalink / raw)
  To: git

Hi,
We have a project on githubie. On our server is the server git (gitosi). Our
server needs to mediate in communication. namely:
People from our team clone the repository from our server. And all four
changes are sending him. When done work in the evening every day someone
comes through ssh to a server and sends on github to change once it gets (so
someone can modify the contents of githubie). We collect these changes on us
from the server.
So the question is how do pośrednczące repository that will work at all push
/ pull from github and at the same time will be from the local computer, it
should work for push / pull.
Thanks for the reply.

--
View this message in context: http://git.661346.n2.nabble.com/Git-server-mediator-tp7138036p7138036.html
Sent from the git mailing list archive at Nabble.com.

^ permalink raw reply

* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users
From: Jeff King @ 2011-12-30 13:54 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: git, Junio C Hamano, Jakub Narebski
In-Reply-To: <4EFD40CF.8000801@tu-clausthal.de>

On Fri, Dec 30, 2011 at 05:40:47AM +0100, Sven Strickroth wrote:

> >> git-core also asks for username using *_ASKPASS, this is the reason why
> >> I implemented it this way. I noticed it when I tried to push to google
> >> code (using https).
> > 
> > I thought that was updated with Peff's series recently?
> 
> So, was this changed? git-core doesn't ask for a username using
> *_ASKPASS helpers anymore?

No, it will. It's only that we will echo characters when using the
terminal prompt. In theory we could have an ASKPASS-style interface that
would would echo characters, but there's no such interface in common
use (i.e., we would have to invent it).

> I talked off-list with Junio and he proposed to use another environment
> variable (e.g. GIT_DIALOG for a different tool) to solve these issues.
> 
> A good way could be to define the GIT_DIALOG-tools to have two
> parameters. First (pass|text|filename|...) with fallback to text, this
> way one can implement a password field, a text field, a file chooser (on
> type filename) and it is still extendable for e.g. directory choosers
> (if we might need that)...

Yes, like that. I think some windowing toolkits already have programs to
provide dialogs from shell scripts. You might look at them for
inspiration on the interface.

For credentials, it would be nice to be able to create a multi-field
dialog, like:

  Username: <text input>
  Password: <text input>
  Remember password? [checkbox]

I was planning to do something custom for credentials as an extension to
the credential helper protocol, but this could also fall under the
heading of a general prompt helper.

-Peff

^ permalink raw reply

* Re: What's the best way to push/fetch replace refs?
From: Christian Couder @ 2011-12-30 14:51 UTC (permalink / raw)
  To: Slawomir Testowy; +Cc: git
In-Reply-To: <CAA61mJgCjt0O5LE5OQ=aNbtUHCZeZdZ=3dXYTNv_AhFi7JXwkA@mail.gmail.com>

On Fri, Dec 30, 2011 at 12:59 PM, Slawomir Testowy
<slawomir.testowy@gmail.com> wrote:
>
> # I also added "fetch = +refs/replace/*:refs/replace/*" to [remote
> "origin"] in "b" and this also seems to work.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Yeah, the simplest is probably to just add "fetch =
+refs/replace/*:refs/replace/*" to the remote(s) you want to get the
replace refs from, and "push = +refs/replace/*:refs/replace/*" to the
remote(s) you want to push the replace refs to.

Regards,
Christian.

^ permalink raw reply

* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users
From: Sven Strickroth @ 2011-12-30 14:53 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Junio C Hamano, Jakub Narebski
In-Reply-To: <20111230135423.GA1684@sigill.intra.peff.net>

Am 30.12.2011 14:54 schrieb Jeff King:
>>> I thought that was updated with Peff's series recently?
>>
>> So, was this changed? git-core doesn't ask for a username using
>> *_ASKPASS helpers anymore?
> 
> No, it will. It's only that we will echo characters when using the
> terminal prompt. In theory we could have an ASKPASS-style interface that
> would would echo characters, but there's no such interface in common
> use (i.e., we would have to invent it).

I also updated the _ASKPASS helper of TortoiseGit so that it only shows
asterisks if "pass" is not contained in the prompt.

> For credentials, it would be nice to be able to create a multi-field
> dialog, like:
> 
>   Username: <text input>
>   Password: <text input>
>   Remember password? [checkbox]
> 
> I was planning to do something custom for credentials as an extension to
> the credential helper protocol, but this could also fall under the
> heading of a general prompt helper.

This might be problematic, because (for git-svn) username and password
are not requested together.

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply

* Re: [PATCH 1/3] expanded hook api with stdio support
From: Joey Hess @ 2011-12-30 17:13 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: git
In-Reply-To: <4EFD88CB.3050403@kdbg.org>

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

Johannes Sixt wrote:
> IMHO, this is overengineered. I don't think that we need something like
> this in the foreseeable future, particularly because such a pipeline or
> multi-hook infrastructure can easily be constructed by the (single) hook
> script itself.

Junio seemed to think this was a good direction to move in and gave some
examples in <7vlipz930t.fsf@alter.siamese.dyndns.org>

Anyway, the minimum cases for run_hook_complex() to support are:

* no stdin, no stdout
* only stdin
* stdin and stdout (needed for tweak-fetch)
* only stdout (perhaps)

The generator and reader members of struct hook allow the caller to
easily specify which of these cases applies to a hook, and also provides
a natural separation of the caller's stdin generation and stdout parsing
code.

That leaves the feeder and data members of struct hook as possible
overengineering. See below regarding the feeder. The data member could
be eliminated and global variables used by callers that need that,
but I prefer designs that don't require global variables.

> > +`run_hook_complex`::
> > +
> > +	Run a hook, with the caller providing its stdin and/or parsing its
> > +	stdout.
> > +	Takes a pointer to a `struct hook` that specifies the details,
> > +	including the name of the hook, any parameters to pass to it,
> > +	and how to handle the stdin and stdout. (See below.)
> > +	If the hook does not exist or is not executable, the return value
> > +	will be zero.
> > +	If it is executable, the hook will be executed and the exit
> > +	status of the hook is returned.
> 
> What is the rationale for these error modes? It is as if a non-existent
> or non-executable hook counts as 'success'. (I'm not saying that this
> would be wrong, I'm just asking.)

They are identical to how run_hook already works.
A non-existant/non-executable hook *is* a valid configuration,
indeed it's the most likely configuration.

> > +/* A feeder that ensures the hook consumes all its stdin. */
> > +int feed_hook_in_full(int handle, struct strbuf *buf, struct hook *hook)
> > +{
> > +	if (write_in_full(handle, buf->buf, buf->len) != buf->len) {
> > +		warning("%s hook failed to consume all its input", hook->name);
> 
> Really? Could there not be any other error condition? The warning would
> be correct only if errno == EPIPE, and this error will be returned only
> if SIGPIPE is ignored. Does this happen anywhere?
> 
> Futhermore, if all data was written to the pipe successfully, there is
> no way to know whether the reader consumed everything.
> 
> IOW, I don't it is worth to make the distinction. However, I do think
> that the parent process must be protected against SIGPIPE.

Yes, this was not thought through, I missed that a write to a pipe can
succeed (due to buffering) despite not being fully consumed.

Dealing with the hook SIGPIPE issue is complicated as Jeff explains in
<20111205214351.GA15029@sigill.intra.peff.net>, and I don't feel I'm the
one to do it. I'm feeling like ripping the "feeder" stuff out of my
patch, and not having my patch change the status quo on this issue.

-- 
see shy jo

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

^ permalink raw reply

* Re: [PATCH 1/3] expanded hook api with stdio support
From: Johannes Sixt @ 2011-12-30 18:04 UTC (permalink / raw)
  To: Joey Hess; +Cc: git
In-Reply-To: <20111230171344.GA9667@gnu.kitenet.net>

Am 30.12.2011 18:13, schrieb Joey Hess:
> Johannes Sixt wrote:
>> IMHO, this is overengineered. I don't think that we need something like
>> this in the foreseeable future, particularly because such a pipeline or
>> multi-hook infrastructure can easily be constructed by the (single) hook
>> script itself.
> 
> Junio seemed to think this was a good direction to move in and gave some
> examples in <7vlipz930t.fsf@alter.siamese.dyndns.org>
> 
> Anyway, the minimum cases for run_hook_complex() to support are:
> 
> * no stdin, no stdout
> * only stdin
> * stdin and stdout (needed for tweak-fetch)
> * only stdout (perhaps)
> 
> The generator and reader members of struct hook allow the caller to
> easily specify which of these cases applies to a hook, and also provides
> a natural separation of the caller's stdin generation and stdout parsing
> code.

But as long as the generator only needs to generate a strbuf *and* only
one hook is run, there is no value to have it as a callback; the caller
can just specify the strbuf itself, run_hook_* does not need to care how
it was generated.

I can see some value in a reader callback to avoid allocating yet
another strbuf.

> ... The data member could
> be eliminated and global variables used by callers that need that,
> but I prefer designs that don't require global variables.

Absolutely.

>>> +	If the hook does not exist or is not executable, the return value
>>> +	will be zero.
>>> +	If it is executable, the hook will be executed and the exit
>>> +	status of the hook is returned.
>>
>> What is the rationale for these error modes? It is as if a non-existent
>> or non-executable hook counts as 'success'. (I'm not saying that this
>> would be wrong, I'm just asking.)
> 
> They are identical to how run_hook already works.
> A non-existant/non-executable hook *is* a valid configuration,
> indeed it's the most likely configuration.

So, it is so that the caller does not itself have to check whether a
hook exists. That may be worth a word in the API documentation.

-- Hannes

^ permalink raw reply

* Re: Git clone - head not pointing to master
From: Jakub Narebski @ 2011-12-30 18:45 UTC (permalink / raw)
  To: Vibin Nair; +Cc: git
In-Reply-To: <4EFD9F11.5030309@volitionlabs.com>

Vibin Nair <vibin@volitionlabs.com> writes:

> Hi,
>   I am a Git beginner. Recently i started working on a codebase (that
> is checked in to a git repository) of our client, who is not very
> aware about how git works.
> The repository has multiple branches such as
> 
> origin/user1
> origin/master
> origin/user2
> 
> Now when i take a clone of this repository : git clone <repo-url>, a
> clone of the repo is created on my machine.
> 
> But when i do a : "git branch" it shows
> 
> user1 *
> 
> which means that the "HEAD" is pointing to the "user1" branch.

Where do you do "git branch"?  In clone or in original repository?
 
> The client told me that all the recent code were being merged into the
> "master" branch and hence the "user1" branch has code which is older
> than "master" branch.
> 
> I had 2 queries :
> 
> 1. Is it possible to make the "HEAD" of the remote point to "master"
> instead of "user1", so that other members of my team get the latest
> code when they try to clone the repository.

If you want "origin/HEAD" to point to "origin/master" remote-tracking
branch, so that "origin" as a branch means "origin/master", you can
use (with modern Git)

  clone$ git remote set-head origin master

or

  clone$ git remote set-head origin

if "master" is now current branch on remote.


If you want to change HEAD i.e. current branch on server (on remote),
you can use

  server$ git checkout master

if it is non-bare repository (not recommended setup), or

  server$ git symbolic-ref HEAD refs/heads/master

or equivalently

  server$ git update-ref --no-deref HEAD refs/heads/master


> 2. Is there any way to safely merge "user1" to "master".

You can check if 'user1' can be fast-forwarded to 'master'
with

  $ git checkout user1
  $ git merge --ff-only master

This would update 'user1' to 'master', but only if 'master' is just
straight advancement of 'user1'.

HTH (Hope That Helps).
-- 
Jakub Narebski

^ permalink raw reply

* 1.7.7.3 wishlist: add --verbose option to git-tag
From: Jari Aalto @ 2011-12-30 23:32 UTC (permalink / raw)
  To: git


In scripts it would be useful if "git tag" would provide option:

    --verbose

As in script:

    git tag --verbose -m "Initial import" upstream/1.0

It would also help if all commands would use similar interface. In "git
tag" case, this would meen relocating:

    -v      =>      -g, --verify-gpg

And reserve these:

    -v, --verbose

Jari

^ permalink raw reply

* Re: [PATCH] stash: Don't fail if work dir contains file named 'HEAD'
From: Jonathon Mah @ 2011-12-31  0:01 UTC (permalink / raw)
  To: Thomas Rast; +Cc: git, Junio C Hamano
In-Reply-To: <8739c28iwh.fsf@thomas.inf.ethz.ch>

Thanks for the feedback, Thomas. I should note, this bug initially came up on #git several days ago.

I've tried to take on all your suggestions; patch v2 imminent.

On 2011-12-30, at 02:15, Thomas Rast wrote:

> Jonathon Mah <me@JonathonMah.com> writes:
>> diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
> [...]
> 
> Other reviewers may want to read these hunks in word diff mode, where it
> is far easier to verify that the functionality tested is a superset:
> 
>  test_expect_success PERL 'saying "n" does nothing' '
>          {+set_state HEAD HEADfile_work HEADfile_index &&+}
>          set_state dir/foo work index &&
>          (echo n; echo {+n; echo+} n) | test_must_fail git stash save -p &&
>          verify_state [-dir/foo work index-]{+HEAD HEADfile_work HEADfile_index+} &&
>          verify_saved_state bar {+&&+}
>  {+      verify_state dir/foo work index+}
>  '

I added a note to the message: In t3904, checks and operations on each file are in the order they'll appear when interactively staging.

That is, "echo y/n; echo y/n; ..." for the three files corresponds to the surrounding checks.



Jonathon Mah
me@JonathonMah.com

^ permalink raw reply

* [PATCHv2] stash: Don't fail if work dir contains file named 'HEAD'
From: Jonathon Mah @ 2011-12-31  0:14 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Thomas Rast

When performing a plain "git stash" (without --patch), git-diff would fail
with "fatal: ambiguous argument 'HEAD': both revision and filename". The
output was piped into git-update-index, masking the failed exit status.
The output is now sent to a temporary file (which is cleaned up by
existing code), and the exit status is checked. The "HEAD" arg to the
git-diff invocation has been disambiguated too, of course.

In patch mode, "git stash -p" would fail harmlessly, leaving the working
dir untouched. Interactive adding is fine, but the resulting tree was
diffed with an ambiguous 'HEAD' argument.

Use >foo (no space) when redirecting output.

In t3904, checks and operations on each file are in the order they'll
appear when interactively staging.

In t3905, fix a bug in "stash save --include-untracked -q is quiet": The
redirected stdout file was considered untracked, and so was removed from
the working directory. Use test path helper functions where appropriate.

Signed-off-by: Jonathon Mah <me@JonathonMah.com>
Acked-by: Thomas Rast <trast@student.ethz.ch>
---
Also contains several fixes / changes for tests. Let me know if these
would better belong separately.

 git-stash.sh                       |    7 +++--
 t/t3903-stash.sh                   |   24 ++++++++++++++++++
 t/t3904-stash-patch.sh             |   47 ++++++++++++++++++++++-------------
 t/t3905-stash-include-untracked.sh |   33 +++++++++++++++++-------
 4 files changed, 80 insertions(+), 31 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index c766692..fe4ab28 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -115,7 +115,8 @@ create_stash () {
 			git read-tree --index-output="$TMPindex" -m $i_tree &&
 			GIT_INDEX_FILE="$TMPindex" &&
 			export GIT_INDEX_FILE &&
-			git diff --name-only -z HEAD | git update-index -z --add --remove --stdin &&
+			git diff --name-only -z HEAD -- >"$TMP-stagenames" &&
+			git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
 			git write-tree &&
 			rm -f "$TMPindex"
 		) ) ||
@@ -134,7 +135,7 @@ create_stash () {
 		w_tree=$(GIT_INDEX_FILE="$TMP-index" git write-tree) ||
 		die "$(gettext "Cannot save the current worktree state")"
 
-		git diff-tree -p HEAD $w_tree > "$TMP-patch" &&
+		git diff-tree -p HEAD $w_tree -- >"$TMP-patch" &&
 		test -s "$TMP-patch" ||
 		die "$(gettext "No changes selected")"
 
@@ -491,7 +492,7 @@ drop_stash () {
 		die "$(eval_gettext "\${REV}: Could not drop stash entry")"
 
 	# clear_stash if we just dropped the last stash entry
-	git rev-parse --verify "$ref_stash@{0}" > /dev/null 2>&1 || clear_stash
+	git rev-parse --verify "$ref_stash@{0}" >/dev/null 2>&1 || clear_stash
 }
 
 apply_to_branch () {
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index fcdb182..dbe2ac1 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -601,4 +601,28 @@ test_expect_success 'stash apply shows status same as git status (relative to cu
 	test_cmp expect actual
 '
 
+cat > expect << EOF
+diff --git a/HEAD b/HEAD
+new file mode 100644
+index 0000000..fe0cbee
+--- /dev/null
++++ b/HEAD
+@@ -0,0 +1 @@
++file-not-a-ref
+EOF
+
+test_expect_success 'stash where working directory contains "HEAD" file' '
+	git stash clear &&
+	git reset --hard &&
+	echo file-not-a-ref > HEAD &&
+	git add HEAD &&
+	test_tick &&
+	git stash &&
+	git diff-files --quiet &&
+	git diff-index --cached --quiet HEAD &&
+	test "$(git rev-parse stash^)" = "$(git rev-parse HEAD)" &&
+	git diff stash^..stash > output &&
+	test_cmp output expect
+'
+
 test_done
diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
index 781fd71..70655c1 100755
--- a/t/t3904-stash-patch.sh
+++ b/t/t3904-stash-patch.sh
@@ -7,7 +7,8 @@ test_expect_success PERL 'setup' '
 	mkdir dir &&
 	echo parent > dir/foo &&
 	echo dummy > bar &&
-	git add bar dir/foo &&
+	echo committed > HEAD &&
+	git add bar dir/foo HEAD &&
 	git commit -m initial &&
 	test_tick &&
 	test_commit second dir/foo head &&
@@ -17,47 +18,57 @@ test_expect_success PERL 'setup' '
 	save_head
 '
 
-# note: bar sorts before dir, so the first 'n' is always to skip 'bar'
+# note: order of files with unstaged changes: HEAD bar dir/foo
 
 test_expect_success PERL 'saying "n" does nothing' '
+	set_state HEAD HEADfile_work HEADfile_index &&
 	set_state dir/foo work index &&
-	(echo n; echo n) | test_must_fail git stash save -p &&
-	verify_state dir/foo work index &&
-	verify_saved_state bar
+	(echo n; echo n; echo n) | test_must_fail git stash save -p &&
+	verify_state HEAD HEADfile_work HEADfile_index &&
+	verify_saved_state bar &&
+	verify_state dir/foo work index
 '
 
 test_expect_success PERL 'git stash -p' '
-	(echo n; echo y) | git stash save -p &&
-	verify_state dir/foo head index &&
+	(echo y; echo n; echo y) | git stash save -p &&
+	verify_state HEAD committed HEADfile_index &&
 	verify_saved_state bar &&
+	verify_state dir/foo head index &&
 	git reset --hard &&
 	git stash apply &&
-	verify_state dir/foo work head &&
-	verify_state bar dummy dummy
+	verify_state HEAD HEADfile_work committed &&
+	verify_state bar dummy dummy &&
+	verify_state dir/foo work head
 '
 
 test_expect_success PERL 'git stash -p --no-keep-index' '
-	set_state dir/foo work index &&
+	set_state HEAD HEADfile_work HEADfile_index &&
 	set_state bar bar_work bar_index &&
-	(echo n; echo y) | git stash save -p --no-keep-index &&
-	verify_state dir/foo head head &&
+	set_state dir/foo work index &&
+	(echo y; echo n; echo y) | git stash save -p --no-keep-index &&
+	verify_state HEAD committed committed &&
 	verify_state bar bar_work dummy &&
+	verify_state dir/foo head head &&
 	git reset --hard &&
 	git stash apply --index &&
-	verify_state dir/foo work index &&
-	verify_state bar dummy bar_index
+	verify_state HEAD HEADfile_work HEADfile_index &&
+	verify_state bar dummy bar_index &&
+	verify_state dir/foo work index
 '
 
 test_expect_success PERL 'git stash --no-keep-index -p' '
-	set_state dir/foo work index &&
+	set_state HEAD HEADfile_work HEADfile_index &&
 	set_state bar bar_work bar_index &&
-	(echo n; echo y) | git stash save --no-keep-index -p &&
+	set_state dir/foo work index &&
+	(echo y; echo n; echo y) | git stash save --no-keep-index -p &&
+	verify_state HEAD committed committed &&
 	verify_state dir/foo head head &&
 	verify_state bar bar_work dummy &&
 	git reset --hard &&
 	git stash apply --index &&
-	verify_state dir/foo work index &&
-	verify_state bar dummy bar_index
+	verify_state HEAD HEADfile_work HEADfile_index &&
+	verify_state bar dummy bar_index &&
+	verify_state dir/foo work index
 '
 
 test_expect_success PERL 'none of this moved HEAD' '
diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
index ef44fb2..a5e7e6b 100755
--- a/t/t3905-stash-include-untracked.sh
+++ b/t/t3905-stash-include-untracked.sh
@@ -17,6 +17,7 @@ test_expect_success 'stash save --include-untracked some dirty working directory
 	echo 3 > file &&
 	test_tick &&
 	echo 1 > file2 &&
+	echo 1 > HEAD &&
 	mkdir untracked &&
 	echo untracked >untracked/untracked &&
 	git stash --include-untracked &&
@@ -35,6 +36,13 @@ test_expect_success 'stash save --include-untracked cleaned the untracked files'
 '
 
 cat > expect.diff <<EOF
+diff --git a/HEAD b/HEAD
+new file mode 100644
+index 0000000..d00491f
+--- /dev/null
++++ b/HEAD
+@@ -0,0 +1 @@
++1
 diff --git a/file2 b/file2
 new file mode 100644
 index 0000000..d00491f
@@ -51,14 +59,16 @@ index 0000000..5a72eb2
 +untracked
 EOF
 cat > expect.lstree <<EOF
+HEAD
 file2
 untracked
 EOF
 
 test_expect_success 'stash save --include-untracked stashed the untracked files' '
-	test "!" -f file2 &&
-	test ! -e untracked &&
-	git diff HEAD stash^3 -- file2 untracked >actual &&
+	test_path_is_missing file2 &&
+	test_path_is_missing untracked &&
+	test_path_is_missing HEAD &&
+	git diff HEAD stash^3 -- HEAD file2 untracked >actual &&
 	test_cmp expect.diff actual &&
 	git ls-tree --name-only stash^3: >actual &&
 	test_cmp expect.lstree actual
@@ -75,6 +85,7 @@ git clean --force --quiet
 
 cat > expect <<EOF
  M file
+?? HEAD
 ?? actual
 ?? expect
 ?? file2
@@ -116,10 +127,12 @@ test_expect_success 'stash save --include-untracked dirty index got stashed' '
 
 git reset > /dev/null
 
+# Must direct output somewhere where it won't be considered an untracked file
 test_expect_success 'stash save --include-untracked -q is quiet' '
 	echo 1 > file5 &&
-	git stash save --include-untracked --quiet > output.out 2>&1 &&
-	test ! -s output.out
+	git stash save --include-untracked --quiet > .git/stash-output.out 2>&1 &&
+	test_line_count = 0 .git/stash-output.out &&
+	rm -f .git/stash-output.out
 '
 
 test_expect_success 'stash save --include-untracked removed files' '
@@ -133,7 +146,7 @@ rm -f expect
 
 test_expect_success 'stash save --include-untracked removed files got stashed' '
 	git stash pop &&
-	test ! -f file
+	test_path_is_missing file
 '
 
 cat > .gitignore <<EOF
@@ -155,14 +168,14 @@ test_expect_success 'stash save --include-untracked respects .gitignore' '
 test_expect_success 'stash save -u can stash with only untracked files different' '
 	echo 4 > file4 &&
 	git stash -u &&
-	test "!" -f file4
+	test_path_is_missing file4
 '
 
 test_expect_success 'stash save --all does not respect .gitignore' '
 	git stash -a &&
-	test "!" -f ignored &&
-	test "!" -e ignored.d &&
-	test "!" -f .gitignore
+	test_path_is_missing ignored &&
+	test_path_is_missing ignored.d &&
+	test_path_is_missing .gitignore
 '
 
 test_expect_success 'stash save --all is stash poppable' '
-- 
1.7.8



Jonathon Mah
me@JonathonMah.com

^ permalink raw reply related

* [PATCH] Documentation: read-tree --prefix works with existing subtrees
From: Clemens Buchacher @ 2011-12-31 11:50 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git


Since 34110cd4 (Make 'unpack_trees()' have a separate source and
destination index) it is no longer true that a subdirectory with
the same prefix must not exist.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 Documentation/git-read-tree.txt |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt
index 5375549..a43e874 100644
--- a/Documentation/git-read-tree.txt
+++ b/Documentation/git-read-tree.txt
@@ -83,11 +83,10 @@ OPTIONS
 
 --prefix=<prefix>/::
 	Keep the current index contents, and read the contents
-	of the named tree-ish under the directory at `<prefix>`. The
-	original index file cannot have anything at the path
-	`<prefix>` itself, nor anything in the `<prefix>/`
-	directory.  Note that the `<prefix>/` value must end
-	with a slash.
+	of the named tree-ish under the directory at `<prefix>`.
+	The command will refuse to overwrite entries that already
+	existed in the original index file. Note that the `<prefix>/`
+	value must end with a slash.
 
 --exclude-per-directory=<gitignore>::
 	When running the command with `-u` and `-m` options, the
-- 
1.7.8

^ permalink raw reply related

* [PATCH] Work around sed portability issue in t8006-blame-textconv
From: Ben Walton @ 2011-12-31 13:44 UTC (permalink / raw)
  To: git, gitster; +Cc: Ben Walton

In test 'blame --textconv with local changes' of t8006-blame-textconv,
using /usr/xpg4/bin/sed on Solaris as set by SANE_TOOL_PATH, an
additional newline was added to the output from the 'helper' script
driven by git attributes.

This was noted by sed with a message such as:
sed: Missing newline at end of file zero.bin.

In turn, this was triggering a fatal error from git blame:
fatal: unable to read files to diff

The git blame --textconv stdout was empty as a result of the error
condition above.  This caused the test to fail because the output
value differed from the expected result.

Use perl -p -e instead of sed -e to work around this portability issue
as it will not insert the newline.  This allows the git blame call to
complete at which point the output comparison is successful.

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 t/t8006-blame-textconv.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh
index 4ee42f1..c3c22f7 100755
--- a/t/t8006-blame-textconv.sh
+++ b/t/t8006-blame-textconv.sh
@@ -10,7 +10,7 @@ find_blame() {
 cat >helper <<'EOF'
 #!/bin/sh
 grep -q '^bin: ' "$1" || { echo "E: $1 is not \"binary\" file" 1>&2; exit 1; }
-sed 's/^bin: /converted: /' "$1"
+perl -p -e 's/^bin: /converted: /' "$1"
 EOF
 chmod +x helper
 
-- 
1.7.4.1

^ permalink raw reply related

* How to deal with historic tar-balls
From: nn6eumtr @ 2011-12-31 19:04 UTC (permalink / raw)
  To: git

I have a number of older projects that I want to bring into a git 
repository. They predate a lot of the popular scm systems, so they are 
primarily a collection of tarballs today.

I'm fairly new to git so I have a couple questions related to this:

- What is the best approach for bringing them in? Do I just create a 
repository, then unpack the files, commit them, clean out the directory 
unpack the next tarball, and repeat until everything is loaded?

- Do I need to pay special attention to files that are renamed/removed 
from version to version?

- If the timestamps change on a file but the actual content does not, 
will git treat it as a non-change once it realizes the content hasn't 
changed?

- Last, if after loading the repository I find another version of the 
files that predates those I've loaded, or are intermediate between two 
commits I've already loaded, is there a way to go say that commit B is 
actually the ancestor of commit C? (i.e. a->c becomes a->b->c if you 
were to visualize the commit timeline or do diffs) Or do I just reload 
the tarballs in order to achieve this?

All replies appreciated!

^ permalink raw reply

* Re: [PATCH] Submodules always use a relative path to gitdir
From: Phil Hord @ 2011-12-31 20:31 UTC (permalink / raw)
  To: Antony Male; +Cc: git, gitster, iveqy
In-Reply-To: <1325192426-10103-1-git-send-email-antony.male@gmail.com>

On Thu, Dec 29, 2011 at 4:00 PM, Antony Male <antony.male@gmail.com> wrote:
> This fixes a problem where moving a git repository with checked-out
> submodules would cause a fatal error when commands such as 'git
> submodule update' were run.

Thanks.  I noticed this itch when looking at git-files a few months
ago.  It bothered me, but not enough to fix it;  just enough to note
it as a problem area to avoid in the future.

> An alternative patch would teach git-clone an option to control whether
> an absolute or relative path is used when --separate-git-dir is passed.

I think I like this option better. Did you look at what it would take?

Phil

^ permalink raw reply

* Re: [PATCH] Submodules always use a relative path to gitdir
From: Phil Hord @ 2011-12-31 21:28 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Antony Male, git, iveqy
In-Reply-To: <7vsjk3vw67.fsf@alter.siamese.dyndns.org>

On Thu, Dec 29, 2011 at 5:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Antony Male <antony.male@gmail.com> writes:
>
>> Git submoule clone uses git clone --separate-git-dir to checkout a
>> submodule's git repository into <supermodule>/.git/modules,...
>
> This is misleading. The <superproject>/.git/modules/<name> is the location
> of the $GIT_DIR for the submodule <name>, not the location of its checkout
> at <superproject>/<path> that is outside <superproject>/.git/modules/
> hierarchy.

Yes, so I think a simple s/checkout/clone/ should fix it.

[...]

>> In the submodules scenario, neither the git repository nor the working
>> copy will be moved relative to each other. However, the supermodule may
>> be moved,...
>
> Again, who said that you are allowed to move the superproject directory in
> the first place? I would understand that it might be nicer if it could be
> moved but I haven't thought this through thoroughly yet---there may be
> other side effects from doing so, other than the relativeness of "gitdir".

Previously it was accepted practice to clone a local repo with rsync.
This method continues to work well even with submodules before
git-files became the norm.  But now it breaks because of the absolute
paths.

Similarly, clones on network mounts and portable drives where absolute
paths may change from time to time or machine to machine will also
break now but worked before.

So, who said you were NOT allowed to move the superproject directory
directory in the first place?  It seems natural that you should be
able to do so, especially since the submodules are all contained
within the superproject path.


>> Previously, if git submodule clone was called when the submodule's git
>> repository already existed in <supermodule>/.git/modules, it would
>> simply re-create the submodule's .git file, using a relative path.
>
> ... "to point at the existing <superproject>/.git/modules/<name>".
>
> Overall, I think I can agree with the goal, but the tone of the proposed
> commit log message rubs the reader in a wrong way to see clearly what this
> patch is proposing to do and where its merit lies. It is probably not a
> big deal, and perhaps it may be just the order of explanation.
>
> I would probably explain the goal like this if I were doing this patch,
> without triggering any need for revisionist history bias.
>
>    Recent versions of "git submodule" maintain the submodule <name> at
>    <path> in the superproject using a "separate git-dir" mechanism. The
>    repository data for the submodule is stored in ".git/modules/<name>/"
>    directory of the superproject, and its working tree is created at
>    "<path>/" directory, with "<path>/.git" file pointing at the
>    ".git/modules/<name>/" directory.
>
>    This is so that we can check out an older version of the superproject
>    that does not yet have the submodule <name> anywhere without losing
>    (and later having to re-clone) the submodule repository. Removing

Revisionism nit: the real danger here is that you lose local commits.

>    "<path>" won't lose ".git/modules/<name>", and a different branch that
>    has the submodule at different location in the superproject, say
>    "<path2>", can create "<path2>/" and ".git" in it to point at the same
>    ".git/modules/<name>".

This doesn't explain why one path is absolute and one is relative.
But I don't suppose this is the place for historical documentation
anyway.

>    When instantiating such a submodule, if ".git/modules/<name>/" does
>    not exist in the superproject, the submodule repository needs to be
>    cloned there first. Then we only need to create "<path>" directory,
>    point ".git/modules/<name>/" in the superproject with "<path>/.git",
>    and check out the working tree.
>
>    However, the current code is not structured that way. The codepath to
>    deal with newly cloned submodules uses "git clone --separate-git-dir"
>    and creates "<path>" and "<path>/.git". This can make the resulting
>    submodule working tree at "<path>" different from the codepath for
>    existing submodules. An example of such differences is that this
>    codepath prepares "<path>/.git" with an absolute path, while the
>    normal codepath uses a relative path.

I had to read this three times before I understood it. There are some
minor grammatical nits in it, but also the use of nearness and use of
"path" and "codepath" to mean two unrelated things was misleading me.
Here's my attempt to clean it up:

    However, the current code is not structured that way. The code to
    deal with newly cloned submodules is different from the code to
    checkout a workdir for existing submodules.  The "newly cloned
    submodule" code uses "git clone --separate-git-dir" to create
    "<path>" and "<path>/.git". The "existing submodules" code
    simply creates the "<path>/.git" internally, using a relative path.
    This makes the resulting submodule working tree at "<path>" different
    depending on which code is used.  An example of such differences
    is that the "newly cloned submodule" code prepares "<path>/.git"
    with an absolute path, while the "existing submodules" code
    prepares the same file using a relative path.


> When explained this way, the remedy is quite clear, and the change is more
> forward-looking, isn't it?  If we later start doing more in the codepath
> to deal with existing submodules, your patch may break without having
> extra code to cover the "newly cloned" case, too.


> I further wonder if we can get away without using separate-git-dir option
> in this codepath, though. IOW using
>
>        git clone $quiet -bare ${reference:+"$reference"} "$url" "$gitdir"
>
> might be a better solution.

You may be right about this one.  I still think the addition of a
--relative-path option to 'git-checkout --separate-work-dir' could be
useful and also easier to maintain/describe.

> For example (this relates to the point I mumbled "haven't thought this
> through thoroughly yet"), doesn't the newly cloned repository have
> core.worktree that points at the working tree that records the <path>,
> which would become meaningless when a commit in the superproject that
> binds the submodule at different path <path2>?

Ooh, yes it does.  Maybe that should be fixed in this case too.

Because submodule cloning with a separate work-dir is a special case
of git-files and work-dirs because we know that each is relative
(subordinate) to the superproject path.  Therefore, I think in this
special-case version of the "separate work-dir" scenario, we should
use super-project-relative paths for both cases.

How do we codify this so this functionality is reliably retained by
future developers?  I think moving the code into someplace more
explicit would help, but I haven't looked too deeply at the code.

>  git-submodule.sh |   21 ++++++++-------------
>  1 files changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 3adab93..9a23e9d 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -156,21 +156,16 @@ module_clone()
>                ;;
>        esac
>
> -       if test -d "$gitdir"
> +       if ! test -d "$gitdir"
>        then
> -               mkdir -p "$path"
> -               echo "gitdir: $rel_gitdir" >"$path/.git"
> -               rm -f "$gitdir/index"
> -       else
> -               mkdir -p "$gitdir_base"
> -               if test -n "$reference"
> -               then
> -                       git-clone $quiet "$reference" -n "$url" "$path" --separate-git-dir "$gitdir"
> -               else
> -                       git-clone $quiet -n "$url" "$path" --separate-git-dir "$gitdir"
> -               fi ||
> -               die "$(eval_gettext "Clone of '\$url' into submodule path '\$path' failed")"
> +               git clone $quiet -n ${reference:+"$reference"} \
> +                       --separate-git-dir "$gitdir" "$url" "$path" ||
> +               die "$(eval_gettext "Clone of '\$url' for submodule '\$name' failed")
>        fi
> +
> +       mkdir -p "$path"
> +       echo "gitdir: $rel_gitdir" >"$path/.git"
> +       rm -f "$gitdir/index"
>  }

Doesn't this avoid creating core.worktree in the first place?  I'm ok
with that because I assume it's never used in the submodule scenario,
but I also suspect that assumption could be wrong.  Any concerns?

Phil

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox