Joey Hess <joey@xxxxxxxxxxx> writes:
> From 073b0921bb5988628e7af423924c410f522f403a Mon Sep 17 00:00:00 2001
> From: Joey Hess <joey@xxxxxxxxxxx>
> 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.
> @@ -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
> +~~~~~~~~~~
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).
*/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html