Ramkumar Ramachandra <artagnon@xxxxxxxxx> writes:
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 523d41a..6c485f6 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -37,7 +37,6 @@ static const char * const cherry_pick_usage[] = {
>
> static int edit, no_replay, no_commit, mainline, signoff, allow_ff;
> static enum { REVERT, CHERRY_PICK } action;
> -static struct commit *commit;
I agree that the stated goal of this change is a worthy one.
> @@ -116,11 +115,12 @@ struct commit_message {
> const char *message;
> };
>
> -static int get_message(const char *raw_message, struct commit_message *out)
> +static int get_message(const char *sha1_abbrev, const char *raw_message,
> + struct commit_message *out)
> {
This somehow feels dubious and also goes against the "let's not rely on
global variable commit", no?
This function relied on the global variable and also got information
passed from the caller that this function could have derived from that
global commit object (i.e. raw_message came from commit->buffer).
A logical thing to fix that situation would be to make its signature:
static int get_message(struct commit *commit, struct commit_message *out)
no?
Especially dubious is this part:
> @@ -139,17 +139,14 @@ static int get_message(const char *raw_message, struct commit_message *out)
> if (out->reencoded_message)
> out->message = out->reencoded_message;
>
> - abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
> - abbrev_len = strlen(abbrev);
> -
> subject_len = find_commit_subject(out->message, &subject);
>
> - out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
> + out->parent_label = xmalloc(strlen("parent of ") + DEFAULT_ABBREV +
> strlen("... ") + subject_len + 1);
You moved the call to f-u-a() to the caller, and that is why you have to
pass its return value as an extra parameter. If you passed commit you do
not have to.
Worse yet, the above xmalloc() and memcpy below are wrong, as you are no
longer taking the actual length of the abbreviated object name into
account, like the old code did. f-u-a() is _about_ uniqueness, and you
can get a string longer than the minimum length you gave to it.
> q = out->parent_label;
> q = mempcpy(q, "parent of ", strlen("parent of "));
> out->label = q;
> - q = mempcpy(q, abbrev, abbrev_len);
> + q = mempcpy(q, sha1_abbrev, DEFAULT_ABBREV);
> q = mempcpy(q, "... ", strlen("... "));
> out->subject = q;
> q = mempcpy(q, subject, subject_len);
> @@ -168,8 +165,7 @@ static char *get_encoding(const char *message)
> const char *p = message, *eol;
>
> if (!p)
> - die (_("Could not read commit message of %s"),
> - sha1_to_hex(commit->object.sha1));
> + die (_("BUG: get_encoding() called with NULL"));
Hmm, do we really want to translate this error message?
As the function is file-scope static, I suspect that calling it with NULL
is a programmer error, not data error (i.e. it is not like we might get an
object with NULL here in some repository created by a different version or
implementation of git). Once you got confident (and I hope you will be
before this series hits our tree) that no caller calls this function with
NULL, we might want to demote this to assert(p) or even remove that if
statement altogether.
> @@ -185,25 +181,26 @@ static char *get_encoding(const char *message)
> return NULL;
> }
>
> -static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
> +static void add_message_to_msg(const char *fallback, struct strbuf *msgbuf,
> + const char *message)
> {
> const char *p = message;
> while (*p && (*p != '\n' || p[1] != '\n'))
> p++;
>
> if (!*p)
> - strbuf_addstr(msgbuf, sha1_to_hex(commit->object.sha1));
> + strbuf_addstr(msgbuf, fallback);
Can you explain what the above loop and if statements are doing for me?
It seems to be doing something a lot more than what the name of the
function suggests, which is a bad sign that the helper is not designed
properly with clear semantics in mind. I would probably prefer to see
this function removed, and do this kind of thing in the only caller of
this confused helper function. You could make an even smaller helper
function that implements the logic to determine if there are two
consecutive LFs in a string (seen above), but I suspect that would be just
the matter of strstr(message, "\n\n")
> -static void write_cherry_pick_head(void)
> +static void write_cherry_pick_head(const char *commit_sha1_hex)
> {
> int fd;
> struct strbuf buf = STRBUF_INIT;
>
> - strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
> + strbuf_addf(&buf, "%s\n", commit_sha1_hex);
>
> fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666);
> if (fd < 0)
> @@ -370,7 +367,7 @@ static int run_git_commit(const char *defmsg)
> return run_command_v_opt(args, RUN_GIT_CMD);
> }
>
> -static int do_pick_commit(void)
> +static int do_pick_commit(struct commit *commit)
> {
> unsigned char head[20];
> struct commit *base, *next, *parent;
> @@ -378,6 +375,7 @@ static int do_pick_commit(void)
> struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
> char *defmsg = NULL;
> struct strbuf msgbuf = STRBUF_INIT;
> + const char *sha1_abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
Given the lifetime rule of f-u-a return value, I think it is a bad design
to call it and pass it as an extra parameter to get_message() much later.
--
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