Re: [PATCH] add post-fetch hook

Joey Hess [off-list ref] writes:

From 073b0921bb5988628e7af423924c410f522f403a Mon Sep 17 00:00:00 2001
From: Joey Hess <redacted>
Date: Mon, 26 Dec 2011 10:53:27 -0400
Subject: [PATCH 2/2] add tweak-fetch hook

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.
---

Just a few style things, as this is not a signed-off patch yet.

The underline does not match what is being underlined. Does this format well?

+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 pulled, and `<local-refname>` is a name of a remote-tracking branch,

s/pulled/fetched/; I think. The remainder of the new text seems to use the
right terminology.

+int feed_tweak_fetch_hook (int in, int out, void *data)

No SP between function name and the opening parenthesis of its parameter
list. We have SP after control-flow keywords e.g. "for (;;)" though.

Does this name need to be external (same question to many other new
functions in this patch)?

The "in" parameter seems unused. Does it have to be there for the "feed"
callback of the generic hook driver? As long as it is the "feed" callback,
I think that it just needs to take "out" and no "in", no?

+	for (ref = data; ref; ref = ref->next) {
+		strbuf_addstr(&buf, sha1_to_hex(ref->old_sha1));
+		strbuf_addch(&buf, ' ');
+		strbuf_addstr(&buf, ref->merge ? "merge" : "not-for-merge");
+		strbuf_addch(&buf, ' ');

strbuf_addf()?

But this might be a moot point, as J6t seems to have valid worries on
running functions that allocate memory in general...

+	ret = write_in_full(out, buf.buf, buf.len) != buf.len;
+	if (ret)
+		warning("%s hook failed to consume all its input",
+				tweak_fetch_hook);
+	close(out);

I was hoping that this part would be part of more generic hook driver
infrastructure. Even if we were to take this series before we refactor
existing other hook drivers, in order to avoid duplicated work later, we
could at least start from a right implementation of a generic hook driver
with a single user (which is the "tweak-fetch" hook driver), no?

+struct ref *parse_tweak_fetch_hook_line (char *l, 
+		struct string_list *existing_refs)
+{
+	struct ref *ref = NULL, *peer_ref = NULL;
+	struct string_list_item *peer_item = NULL;
+	char *words[4];
+	int i, word=0;

SP around assingment and initialization "var = val" (throughout this
patch).

+	char *problem;
+
+	for (i=0; l[i]; i++) {

Likewise.

+		if (isspace(l[i])) {
+			l[i]='\0';
+			words[word]=l;
+			l+=i+1;
+			i=0;
+			word++;
+			if (word > 3) {
+				problem="too many words";
+				goto unparsable;
+			}
+		}
+	}
+	if (word < 3) {
+		problem="not enough words";
+		goto unparsable;
+	}

Perhaps loop for up-to ARRAY_SIZE(words) times and use strchr()?

+	if (strcmp(words[1], "merge") == 0) {

We tend to say "if (!strcmp(...))" instead.

+		ref->merge=1;
+	}
+	else if (strcmp(words[1], "not-for-merge") != 0) {

Likewise.

+struct refs_result read_tweak_fetch_hook (int in) {

Opening brace at column 1 of the next line.

+			if (prevref) {
+				prevref->next=ref;
+				prevref=ref;
+			}
+			else {

	if (...) {
		...
	} else {
		...
	}

+/* The 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.
+ */

	/*
         * We write our multi-line comments
         * like this (applies to a few other comments
         * in this patch).
         */