* Re: [PATCH v3 02/11] commit, cherry-pick -s: remove broken support for multiline rfc2822 fields
From: Jonathan Nieder @ 2013-01-28 1:29 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, pclouds, gitster, Brandon Casey
In-Reply-To: <1359335515-13818-3-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> sequencer.c | 6 ------
> 1 file changed, 6 deletions(-)
Nice simplification.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 06/10] sequencer.c: teach append_signoff how to detect duplicate s-o-b
From: Junio C Hamano @ 2013-01-28 2:06 UTC (permalink / raw)
To: Brandon Casey; +Cc: Jonathan Nieder, pclouds, git, Brandon Casey
In-Reply-To: <CA+sFfMcyupLGPt8-3PWMzwgGR3zrm4ZfA-7KwBh5VkMXmhuTKQ@mail.gmail.com>
Brandon Casey <drafnel@gmail.com> writes:
> On Tue, Jan 22, 2013 at 12:38 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Brandon Casey wrote:
>>
>>> Teach append_signoff how to detect a duplicate s-o-b in the commit footer.
>>> This is in preparation to unify the append_signoff implementations in
>>> log-tree.c and sequencer.c.
>> [...]
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -1082,9 +1101,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)
>>> return pick_commits(todo_list, opts);
>>> }
>>>
>>> -void append_signoff(struct strbuf *msgbuf, int ignore_footer)
>>> +void append_signoff(struct strbuf *msgbuf, int ignore_footer, int no_dup_sob)
>>
>> Isn't the behavior of passing '1' here just a bug in "format-patch -s"?
>
> I think that is an open question.
Yes. as I said in a previous review round, I think it was a mistake
that format-patch chose to pay attention to any S-o-b in the patch
trail, not only the last one, and we may want to correcct it once
this series solidifies as a separate "bugfix" change on top.
^ permalink raw reply
* Re: [PATCH v3 04/11] t/t3511: add some tests of 'cherry-pick -s' functionality
From: Jonathan Nieder @ 2013-01-28 2:08 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, pclouds, gitster, Brandon Casey
In-Reply-To: <1359335515-13818-5-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> --- /dev/null
> +++ b/t/t3511-cherry-pick-x.sh
> @@ -0,0 +1,111 @@
> +#!/bin/sh
> +
> +test_description='Test cherry-pick -x and -s'
> +
> +. ./test-lib.sh
> +
> +pristine_detach () {
> + git cherry-pick --quit &&
> + git checkout -f "$1^0" &&
> + git read-tree -u --reset HEAD &&
> + git clean -d -f -f -q -x
> +}
Some day this should move to test-lib-functions.sh. Not relevant
for this patch, though.
[...]
> +test_expect_failure 'cherry-pick -s inserts blank line after non-conforming footer' '
> + pristine_detach initial &&
> + git cherry-pick -s mesg-broken-footer &&
> + cat <<-EOF >expect &&
> + $mesg_broken_footer
> +
> + Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
This bug is old. When c1e01b0c (commit: More generous accepting of
RFC-2822 footer lines, 2009-10-28) added more precise parsing of the
RFC2822 footer when deciding whether to add a newline before a new
signoff, it forgot to change the "sign-off already present" case to
match.
Thanks for the test. The rest of the tests in this file also look
good, of course.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 0/4] Auto-generate mergetool lists
From: Junio C Hamano @ 2013-01-28 2:08 UTC (permalink / raw)
To: David Aguilar; +Cc: git, John Keeping
In-Reply-To: <1359334346-5879-1-git-send-email-davvid@gmail.com>
I think our works crossed, while I was tweaking the previous series
to push out as part of 'pu' you were already rerolling. Could you
compare this series with what I pushed out and see if anything you
missed? I think I fixed the (a && b || c && d) issue in the version
I pushed out, but it is still there in this series.
^ permalink raw reply
* Re: [PATCH v2 4/4] doc: Generate a list of valid merge tools
From: Junio C Hamano @ 2013-01-28 2:14 UTC (permalink / raw)
To: David Aguilar; +Cc: git, John Keeping
In-Reply-To: <1359334346-5879-5-git-send-email-davvid@gmail.com>
David Aguilar <davvid@gmail.com> writes:
> Use the show_tool_names() function to build lists of all
> the built-in tools supported by difftool and mergetool.
> This frees us from needing to update the documentation
> whenever a new tool is added.
>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> Adjusted to use show_tool_names() and reworked the makefile dependencies.
> I could use another set of eyes on the Makefile..
Looks good from a quick scan and comparison with the previous
round. Thanks for a quick reroll.
> Documentation/.gitignore | 1 +
> Documentation/Makefile | 22 ++++++++++++++++++++--
> Documentation/diff-config.txt | 13 +++++++------
> Documentation/merge-config.txt | 12 ++++++------
> git-mergetool--lib.sh | 3 ++-
> 5 files changed, 36 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/.gitignore b/Documentation/.gitignore
> index d62aebd..2c8b2d6 100644
> --- a/Documentation/.gitignore
> +++ b/Documentation/.gitignore
> @@ -9,4 +9,5 @@ gitman.info
> howto-index.txt
> doc.dep
> cmds-*.txt
> +mergetools-*.txt
> manpage-base-url.xsl
> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index 267dfe1..834ec25 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -202,7 +202,11 @@ install-html: html
> #
> # Determine "include::" file references in asciidoc files.
> #
> -doc.dep : $(wildcard *.txt) build-docdep.perl
> +docdep_prereqs = \
> + mergetools-list.made $(mergetools_txt) \
> + cmd-list.made $(cmds_txt)
> +
> +doc.dep : $(docdep_prereqs) $(wildcard *.txt) build-docdep.perl
> $(QUIET_GEN)$(RM) $@+ $@ && \
> $(PERL_PATH) ./build-docdep.perl >$@+ $(QUIET_STDERR) && \
> mv $@+ $@
> @@ -226,13 +230,27 @@ cmd-list.made: cmd-list.perl ../command-list.txt $(MAN1_TXT)
> $(PERL_PATH) ./cmd-list.perl ../command-list.txt $(QUIET_STDERR) && \
> date >$@
>
> +mergetools_txt = mergetools-diff.txt mergetools-merge.txt
> +
> +$(mergetools_txt): mergetools-list.made
> +
> +mergetools-list.made: ../git-mergetool--lib.sh $(wildcard ../mergetools/*)
> + $(QUIET_GEN)$(RM) $@ && \
> + $(SHELL_PATH) -c 'MERGE_TOOLS_DIR=../mergetools && \
> + . ../git-mergetool--lib.sh && \
> + show_tool_names can_diff "* "' > mergetools-diff.txt && \
> + $(SHELL_PATH) -c 'MERGE_TOOLS_DIR=../mergetools && \
> + . ../git-mergetool--lib.sh && \
> + show_tool_names can_merge "* "' > mergetools-merge.txt && \
> + date > $@
> +
> clean:
> $(RM) *.xml *.xml+ *.html *.html+ *.1 *.5 *.7
> $(RM) *.texi *.texi+ *.texi++ git.info gitman.info
> $(RM) *.pdf
> $(RM) howto-index.txt howto/*.html doc.dep
> $(RM) technical/api-*.html technical/api-index.txt
> - $(RM) $(cmds_txt) *.made
> + $(RM) $(cmds_txt) $(mergetools_txt) *.made
> $(RM) manpage-base-url.xsl
>
> $(MAN_HTML): %.html : %.txt
> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
> index 67a90a8..7c968d1 100644
> --- a/Documentation/diff-config.txt
> +++ b/Documentation/diff-config.txt
> @@ -132,9 +132,10 @@ diff.<driver>.cachetextconv::
> conversion outputs. See linkgit:gitattributes[5] for details.
>
> diff.tool::
> - The diff tool to be used by linkgit:git-difftool[1]. This
> - option overrides `merge.tool`, and has the same valid built-in
> - values as `merge.tool` minus "tortoisemerge" and plus
> - "kompare". Any other value is treated as a custom diff tool,
> - and there must be a corresponding `difftool.<tool>.cmd`
> - option.
> + Controls which diff tool is used by linkgit:git-difftool[1].
> + This variable overrides the value configured in `merge.tool`.
> + The list below shows the valid built-in values.
> + Any other value is treated as a custom diff tool and requires
> + that a corresponding difftool.<tool>.cmd variable is defined.
> +
> +include::mergetools-diff.txt[]
> diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
> index 861bd6f..5f40e71 100644
> --- a/Documentation/merge-config.txt
> +++ b/Documentation/merge-config.txt
> @@ -52,12 +52,12 @@ merge.stat::
> at the end of the merge. True by default.
>
> merge.tool::
> - Controls which merge resolution program is used by
> - linkgit:git-mergetool[1]. Valid built-in values are: "araxis",
> - "bc3", "diffuse", "ecmerge", "emerge", "gvimdiff", "kdiff3", "meld",
> - "opendiff", "p4merge", "tkdiff", "tortoisemerge", "vimdiff"
> - and "xxdiff". Any other value is treated is custom merge tool
> - and there must be a corresponding mergetool.<tool>.cmd option.
> + Controls which merge tool is used by linkgit:git-mergetool[1].
> + The list below shows the valid built-in values.
> + Any other value is treated as a custom merge tool and requires
> + that a corresponding mergetool.<tool>.cmd variable is defined.
> +
> +include::mergetools-merge.txt[]
>
> merge.verbosity::
> Controls the amount of output shown by the recursive merge
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index fe068f6..f665bee 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -1,6 +1,7 @@
> #!/bin/sh
> # git-mergetool--lib is a library for common merge tool functions
> -MERGE_TOOLS_DIR=$(git --exec-path)/mergetools
> +
> +: ${MERGE_TOOLS_DIR=$(git --exec-path)/mergetools}
>
> mode_ok () {
> diff_mode && can_diff ||
^ permalink raw reply
* Re: [PATCH v2 0/4] Auto-generate mergetool lists
From: David Aguilar @ 2013-01-28 2:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Keeping
In-Reply-To: <7v38xm12kk.fsf@alter.siamese.dyndns.org>
On Sun, Jan 27, 2013 at 6:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I think our works crossed, while I was tweaking the previous series
> to push out as part of 'pu' you were already rerolling. Could you
> compare this series with what I pushed out and see if anything you
> missed? I think I fixed the (a && b || c && d) issue in the version
> I pushed out, but it is still there in this series.
Ah, I see.
I can add the addition of preamble for use by show_tool_help()
as a follow up along with using a here-doc when printing.
The other diff is the Makefile dependencies.
I currently have this diff against da/mergetool-docs:
diff --git a/Documentation/Makefile b/Documentation/Makefile
index f595d26..834ec25 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -202,7 +202,11 @@ install-html: html
#
# Determine "include::" file references in asciidoc files.
#
-doc.dep : $(wildcard *.txt) build-docdep.perl
+docdep_prereqs = \
+ mergetools-list.made $(mergetools_txt) \
+ cmd-list.made $(cmds_txt)
+
+doc.dep : $(docdep_prereqs) $(wildcard *.txt) build-docdep.perl
$(QUIET_GEN)$(RM) $@+ $@ && \
$(PERL_PATH) ./build-docdep.perl >$@+ $(QUIET_STDERR) && \
mv $@+ $@
I'll send three follow-up patches. - here-doc, preamble stuff,
and Makefile deps, based on what's currently in pu.
--
David
^ permalink raw reply related
* Re: [PATCH v3 05/11] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer
From: Jonathan Nieder @ 2013-01-28 2:21 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, pclouds, gitster, Brandon Casey
In-Reply-To: <1359335515-13818-6-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> Let's detect "(cherry picked from...)" as part of the footer so that we
> will produce this:
>
> Signed-off-by: A U Thor <author@example.com>
> (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
> Signed-off-by: C O Mmitter <committer@example.com>
>
> instead of this:
>
> Signed-off-by: A U Thor <author@example.com>
> (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
>
> Signed-off-by: C O Mmitter <committer@example.com>
Here's the tweak I suggested last time. I think its behavior is
slightly better in the "ends with incomplete line" case because it
limits the characters examined by is_rfc2822_line() and
is_cherry_picked_from_line() not to include buf[len] (which would
presumably sometimes be '\0').
diff --git i/sequencer.c w/sequencer.c
index 0b5cd18c..fa29c7c5 100644
--- i/sequencer.c
+++ w/sequencer.c
@@ -1043,9 +1043,7 @@ static int is_cherry_picked_from_line(const char *buf, int len)
/*
* We only care that it looks roughly like (cherry picked from ...)
*/
- return !prefixcmp(buf, cherry_picked_prefix) &&
- (buf[len - 1] == ')' ||
- (buf[len - 1] == '\n' && buf[len - 2] == ')'));
+ return !prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
}
static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
@@ -1072,8 +1070,8 @@ static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
; /* do nothing */
k++;
- if (!(is_rfc2822_line(buf + i, k - i) ||
- is_cherry_picked_from_line(buf + i, k - i)))
+ if (!is_rfc2822_line(buf + i, k - i - 1) &&
+ !is_cherry_picked_from_line(buf + i, k - i - 1))
return 0;
}
return 1;
^ permalink raw reply related
* Re: [PATCH v2 0/4] Auto-generate mergetool lists
From: Junio C Hamano @ 2013-01-28 2:27 UTC (permalink / raw)
To: David Aguilar; +Cc: git, John Keeping
In-Reply-To: <CAJDDKr7sQKDNPpaFJi+en479TF=aLXL2pvXODeo6CC3xD1KMGw@mail.gmail.com>
David Aguilar <davvid@gmail.com> writes:
> On Sun, Jan 27, 2013 at 6:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> I think our works crossed, while I was tweaking the previous series
>> to push out as part of 'pu' you were already rerolling. Could you
>> compare this series with what I pushed out and see if anything you
>> missed? I think I fixed the (a && b || c && d) issue in the version
>> I pushed out, but it is still there in this series.
>
> Ah, I see.
>
> I can add the addition of preamble for use by show_tool_help()
> as a follow up along with using a here-doc when printing.
I think the progression of the series is just fine as-is with the
new series you posted (I didn't amend the old one with all the
suggestions I made in the review, just only with the more important
ones that would affect correctness, so please consider that the
changes you have in this new round that I didn't have in 'pu' are
good ones to keep.
^ permalink raw reply
* Re: [PATCH v3 06/11] sequencer.c: always separate "(cherry picked from" from commit body
From: Jonathan Nieder @ 2013-01-28 2:34 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, pclouds, gitster, Brandon Casey
In-Reply-To: <1359335515-13818-7-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -20,6 +20,67 @@
[...]
> static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
[...]
> + /* require at least one blank line */
> + if (!last_char_was_nl || buf[i] != '\n')
> + return 0;
Makes sense. append_signoff already added a blank line after a
one-line message
foo: bar
because of e5138436 (builtin-commit.c: fix logic to omit empty line
before existing footers, 2009-11-06) but the logic to do so was in the
wrong place and it didn't handle its ill-formatted cousin
foo: bar
baz: qux
[...]
> @@ -497,6 +558,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
> }
>
> if (opts->record_origin) {
> + if (!has_conforming_footer(&msgbuf, 0))
> + strbuf_addch(&msgbuf, '\n');
What should this do in the case of an entirely blank message?
(Maybe that's too insane a case to worry about.)
[...]
> --- a/t/t3511-cherry-pick-x.sh
> +++ b/t/t3511-cherry-pick-x.sh
> @@ -81,6 +94,19 @@ test_expect_failure 'cherry-pick -s inserts blank line after non-conforming foot
> test_cmp expect actual
> '
>
> +test_expect_success 'cherry-pick -x inserts blank line when conforming footer not found' '
Yay. :)
Thanks for a clear and well thought-out patch with thorough tests.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 0/4] Auto-generate mergetool lists
From: David Aguilar @ 2013-01-28 2:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Keeping
In-Reply-To: <7vr4l6yrd3.fsf@alter.siamese.dyndns.org>
On Sun, Jan 27, 2013 at 6:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>> On Sun, Jan 27, 2013 at 6:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> I think our works crossed, while I was tweaking the previous series
>>> to push out as part of 'pu' you were already rerolling. Could you
>>> compare this series with what I pushed out and see if anything you
>>> missed? I think I fixed the (a && b || c && d) issue in the version
>>> I pushed out, but it is still there in this series.
>>
>> Ah, I see.
>>
>> I can add the addition of preamble for use by show_tool_help()
>> as a follow up along with using a here-doc when printing.
>
> I think the progression of the series is just fine as-is with the
> new series you posted (I didn't amend the old one with all the
> suggestions I made in the review, just only with the more important
> ones that would affect correctness, so please consider that the
> changes you have in this new round that I didn't have in 'pu' are
> good ones to keep.
Okay, cool, so no need to reroll, ya?
The one thing missing in my latest series was the fixed mode_ok()
which you corrected in cfb611b34089a0b5794f4ec453289a4764d94050.
Let me know if there's anything else I should send out or splice together.
John, I didn't completely address your question about keeping
the sort and prefix in show_tool_help() but I can stop poking at
it now in case you want to start looking at what it would take
to get custom tools listed in the --tool-help output.
--
David
^ permalink raw reply
* Re: [PATCH v3 05/11] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer
From: Jonathan Nieder @ 2013-01-28 2:51 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, pclouds, gitster, Brandon Casey
In-Reply-To: <20130128022148.GG8206@elie.Belkin>
Jonathan Nieder wrote:
> Here's the tweak I suggested last time. I think its behavior is
> slightly better in the "ends with incomplete line" case because it
> limits the characters examined by is_rfc2822_line() and
> is_cherry_picked_from_line() not to include buf[len] (which would
> presumably sometimes be '\0').
Whoops, that revealed a subtlety --- the '\n' or '\0' is what prevents
exiting the loop in is_rfc2822_line when the line does not contain a
colon. Here's a corrected version of the tweak, that should actually
work. :)
diff --git i/sequencer.c w/sequencer.c
index 0b5cd18c..108ea27b 100644
--- i/sequencer.c
+++ w/sequencer.c
@@ -1029,13 +1029,11 @@ static int is_rfc2822_line(const char *buf, int len)
for (i = 0; i < len; i++) {
int ch = buf[i];
if (ch == ':')
+ return 1;
+ if (!isalnum(ch) && ch != '-')
break;
- if (isalnum(ch) || (ch == '-'))
- continue;
- return 0;
}
-
- return 1;
+ return 0;
}
static int is_cherry_picked_from_line(const char *buf, int len)
@@ -1043,9 +1041,7 @@ static int is_cherry_picked_from_line(const char *buf, int len)
/*
* We only care that it looks roughly like (cherry picked from ...)
*/
- return !prefixcmp(buf, cherry_picked_prefix) &&
- (buf[len - 1] == ')' ||
- (buf[len - 1] == '\n' && buf[len - 2] == ')'));
+ return !prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
}
static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
@@ -1072,8 +1068,8 @@ static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
; /* do nothing */
k++;
- if (!(is_rfc2822_line(buf + i, k - i) ||
- is_cherry_picked_from_line(buf + i, k - i)))
+ if (!is_rfc2822_line(buf + i, k - i - 1) &&
+ !is_cherry_picked_from_line(buf + i, k - i - 1))
return 0;
}
return 1;
^ permalink raw reply related
* Re: [PATCH v2 0/4] Auto-generate mergetool lists
From: Junio C Hamano @ 2013-01-28 2:53 UTC (permalink / raw)
To: David Aguilar; +Cc: git, John Keeping
In-Reply-To: <CAJDDKr75K3RGgU79nrznbpjQMLQGkDs=W8XEofURNsS1X1bvjg@mail.gmail.com>
David Aguilar <davvid@gmail.com> writes:
> Okay, cool, so no need to reroll, ya?
It was more like "please don't switch to incremental yet"; I tweaked
the mode_ok in your v2 and pushed out the result on 'pu' again.
There may later be comments from others that make us realize some
patches need to be rerolled, but nothing from me for now.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 07/11] sequencer.c: teach append_signoff how to detect duplicate s-o-b
From: Jonathan Nieder @ 2013-01-28 3:08 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, pclouds, gitster, Brandon Casey
In-Reply-To: <1359335515-13818-8-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> Teach append_signoff how to detect a duplicate s-o-b in the commit footer.
This replaces the previous (slightly broken) logic that checked
whether the sign-off to be appended would be redundant and puts the
fixed logic further down the call-chain next to the rest of footer
parsing.
I am not thrilled with the
0 = no rfc-style footer
1 = rfc-style footer, no sign-off found
2 = rfc-style footer, sign-off found but not as last field
3 = rfc-style footer, sign-off found as last field
convention but since it's local to sequencer.c and this logic to scan
all lines for the sign-off can probably be ripped out soon I don't
mind.
The general direction is good and the execution looks obviously
correct.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCH v3 07/11] sequencer.c: teach append_signoff how to detect duplicate s-o-b
From: Jonathan Nieder @ 2013-01-28 3:14 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, pclouds, gitster, Brandon Casey
In-Reply-To: <1359335515-13818-8-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> --- a/sequencer.c
> +++ b/sequencer.c
[...]
> @@ -1096,10 +1117,16 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer)
> strbuf_addch(&sob, '\n');
> for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
> ; /* do nothing */
> - if (prefixcmp(msgbuf->buf + i, sob.buf)) {
> - if (!i || !has_conforming_footer(msgbuf, ignore_footer))
> - strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
> - strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);
> - }
> +
> + if (i)
> + has_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);
Is this "if (i)" test needed? I'd expect that if the message has no newlines,
has_conforming_footer() would notice that and return 0.
^ permalink raw reply
* Re: [PATCH v3 08/11] sequencer.c: teach append_signoff to avoid adding a duplicate newline
From: Jonathan Nieder @ 2013-01-28 3:23 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, pclouds, gitster, Brandon Casey
In-Reply-To: <1359335515-13818-9-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> Teach append_signoff to detect whether a blank line exists at the position
> that the signed-off-by line will be added, and avoid adding an additional
> one if one already exists. This is necessary to allow format-patch to add a
> s-o-b to a patch with no commit message without adding an extra newline.
I assume this means you're preserving historical behavior when adding
a sign-off to a commit with empty description (which is a good thing),
but what is that behavior? Is it a deliberate choice or something
that developed by default?
[...]
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1118,11 +1118,15 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
> for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
> ; /* do nothing */
>
> - if (i)
> - has_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);
> -
> - if (!has_footer)
> - strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
> + if (msgbuf->buf[i] != '\n') {
> + if (i)
> + has_footer = has_conforming_footer(msgbuf, &sob,
> + ignore_footer);
> +
> + if (!has_footer)
> + strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
> + "\n", 1);
> + }
>
> if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
This is too much nesting for my small mind to keep track of. Am I
correct in imagining the effect is the same as the following?
has_footer = has_conforming_footer(...)
if (!has_footer && msgbuf->buf[i] != '\n')
strbuf_splice(...); /* add blank line */
if (has_footer != 3 && ...
^ permalink raw reply
* Re: [PATCH v3 09/11] t4014: more tests about appending s-o-b lines
From: Jonathan Nieder @ 2013-01-28 3:31 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, pclouds, gitster, Brandon Casey
In-Reply-To: <1359335515-13818-10-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
[...]
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1021,4 +1021,246 @@ test_expect_success 'cover letter using branch description (6)' '
> grep hello actual >/dev/null
> '
>
> +append_signoff()
> +{
> + C=`git commit-tree HEAD^^{tree} -p HEAD` &&
> + git format-patch --stdout --signoff ${C}^..${C} |
> + tee append_signoff.patch |
> + sed -n "1,/^---$/p" |
> + grep -n -E "^Subject|Sign|^$"
> +}
Is "grep -n" portable? I didn't find any uses of it elsewhere in the
testsuite.
Style: checking exit status from format-patch, avoiding sed|grep pipeline:
C=$(git commit-tree HEAD^ -p HEAD) &&
git format-patch --stdout --signoff $C^..$C >append_signoff.patch &&
awk '
/^---$/ { exit; }
/^Subject/ || /^Sign/ || /^$/ { print NR ":" $0 }
' <append_signoff.patch >actual
^ permalink raw reply
* Re: [PATCH v3 10/11] format-patch: update append_signoff prototype
From: Jonathan Nieder @ 2013-01-28 3:35 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, pclouds, gitster, Brandon Casey
In-Reply-To: <1359335515-13818-11-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> This is a preparation step for merging with append_signoff from
> sequencer.c
Avoids a small unfreed allocation, too. Neat.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
(On the other hand, this means more malloc churn when running
"format-patch -s" on a long series of patches, but I don't think
anyone will mind.)
[...]
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1193,16 +1192,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> rev.subject_prefix = strbuf_detach(&sprefix, NULL);
> }
>
> - if (do_signoff) {
> - const char *committer;
> - const char *endpos;
> - committer = git_committer_info(IDENT_STRICT);
sequencer.c uses fmt_name() which uses IDENT_STRICT, too. Phew.
^ permalink raw reply
* Re: [PATCH v3 11/11] Unify appending signoff in format-patch, commit and sequencer
From: Jonathan Nieder @ 2013-01-28 3:39 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, pclouds, gitster, Brandon Casey
In-Reply-To: <1359335515-13818-12-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> --- a/log-tree.c
> +++ b/log-tree.c
[...]
> @@ -208,94 +207,6 @@ void show_decorations(struct rev_info *opt, struct commit *commit)
> putchar(')');
> }
>
> -/*
> - * Search for "^[-A-Za-z]+: [^@]+@" pattern. It usually matches
> - * Signed-off-by: and Acked-by: lines.
> - */
That's stricter than the test from sequencer.c. Maybe it's worth
stealing to avoid false positives?
^ permalink raw reply
* Re: [PATCH v3 00/11] unify appending of sob
From: Jonathan Nieder @ 2013-01-28 3:48 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, pclouds, gitster
In-Reply-To: <1359335515-13818-1-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> Round 3.
Thanks for a pleasant read. My only remaining observations are
cosmetic, except for a portability question in Duy's test script, a
small behavior change when the commit message ends with an
RFC2822-style header with no trailing newline and the possibility of
tightening the pattern in sequencer.c to match the strictness of
format-patch (which could easily wait for a later patch).
Jonathan
^ permalink raw reply
* Updating shared ref from remote helper, or fetch hook
From: Jed Brown @ 2013-01-28 5:19 UTC (permalink / raw)
To: git
I'm working on an hg remote helper that uses git notes for the sha1
revision, so that git users can more easily refer to specific commits
when communicating with hg users. Since there may be multiple
concurrent fast-import streams, I write the notes to a private ref
(refs/notes/hg-REMOTE), to be merged eventually using
git notes --ref hg merge hg-REMOTE*
There will never be conflicts because each hg commit translates to a
unique git commit, thus even if multiple concurrent remotes process the
same commit, the corresponding note will match.
Unfortunately, I couldn't find a safe way to get this run after a fetch
or clone. Of course I can ask the user to arrange to have this command
run, but it would be a better interface to have it run automatically
since it is a natural responsibility of the remote helper. Am I missing
a way to do this or a viable alternative approach?
^ permalink raw reply
* Re: [PATCH v3 05/11] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer
From: Brandon Casey @ 2013-01-28 5:38 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, pclouds, gitster, Brandon Casey
In-Reply-To: <20130128025140.GI8206@elie.Belkin>
On Sun, Jan 27, 2013 at 6:51 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Jonathan Nieder wrote:
>
>> Here's the tweak I suggested last time. I think its behavior is
>> slightly better in the "ends with incomplete line" case because it
>> limits the characters examined by is_rfc2822_line() and
>> is_cherry_picked_from_line() not to include buf[len] (which would
>> presumably sometimes be '\0').
>
> Whoops, that revealed a subtlety --- the '\n' or '\0' is what prevents
> exiting the loop in is_rfc2822_line when the line does not contain a
> colon. Here's a corrected version of the tweak, that should actually
> work. :)
>
> diff --git i/sequencer.c w/sequencer.c
> index 0b5cd18c..108ea27b 100644
> --- i/sequencer.c
> +++ w/sequencer.c
> @@ -1029,13 +1029,11 @@ static int is_rfc2822_line(const char *buf, int len)
> for (i = 0; i < len; i++) {
> int ch = buf[i];
> if (ch == ':')
> + return 1;
> + if (!isalnum(ch) && ch != '-')
> break;
> - if (isalnum(ch) || (ch == '-'))
> - continue;
> - return 0;
> }
> -
> - return 1;
> + return 0;
> }
>
> static int is_cherry_picked_from_line(const char *buf, int len)
> @@ -1043,9 +1041,7 @@ static int is_cherry_picked_from_line(const char *buf, int len)
> /*
> * We only care that it looks roughly like (cherry picked from ...)
> */
> - return !prefixcmp(buf, cherry_picked_prefix) &&
> - (buf[len - 1] == ')' ||
> - (buf[len - 1] == '\n' && buf[len - 2] == ')'));
> + return !prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
> }
>
> static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
> @@ -1072,8 +1068,8 @@ static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
> ; /* do nothing */
> k++;
>
> - if (!(is_rfc2822_line(buf + i, k - i) ||
> - is_cherry_picked_from_line(buf + i, k - i)))
> + if (!is_rfc2822_line(buf + i, k - i - 1) &&
> + !is_cherry_picked_from_line(buf + i, k - i - 1))
> return 0;
> }
> return 1;
Looks good to me.
-Brandon
^ permalink raw reply
* Re: [PATCH v3 09/11] t4014: more tests about appending s-o-b lines
From: Junio C Hamano @ 2013-01-28 5:42 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Brandon Casey, git, pclouds, Brandon Casey
In-Reply-To: <20130128033146.GM8206@elie.Belkin>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Brandon Casey wrote:
>
> [...]
>> --- a/t/t4014-format-patch.sh
>> +++ b/t/t4014-format-patch.sh
>> @@ -1021,4 +1021,246 @@ test_expect_success 'cover letter using branch description (6)' '
>> grep hello actual >/dev/null
>> '
>>
>> +append_signoff()
>> +{
>> + C=`git commit-tree HEAD^^{tree} -p HEAD` &&
>> + git format-patch --stdout --signoff ${C}^..${C} |
>> + tee append_signoff.patch |
>> + sed -n "1,/^---$/p" |
>> + grep -n -E "^Subject|Sign|^$"
>> +}
>
> Is "grep -n" portable? I didn't find any uses of it elsewhere in the
> testsuite.
Yes, "-n" is in POSIX. Even though we use it ourselves, "git grep"
supports it, too.
Any Emacs user would scream if their platform "grep" does not
support it, as it will make M-x grep (or grep-find) useless.
> Style: checking exit status from format-patch, avoiding sed|grep pipeline:
>
> C=$(git commit-tree HEAD^ -p HEAD) &&
> git format-patch --stdout --signoff $C^..$C >append_signoff.patch &&
> awk '
> /^---$/ { exit; }
> /^Subject/ || /^Sign/ || /^$/ { print NR ":" $0 }
> ' <append_signoff.patch >actual
Yeah, awk/perl would be fine, too, and it is good that you pointed
out that the original was losing the exit status from format-patch.
Thanks.
^ permalink raw reply
* Re: [PATCH 6/7] read-cache: refuse to create index referring to external objects
From: Duy Nguyen @ 2013-01-28 5:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jens Lehmann
In-Reply-To: <7vpq0t3x60.fsf@alter.siamese.dyndns.org>
On Sat, Jan 26, 2013 at 2:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
> This is not a tangent, but if you want to go this "forbid making our
> repository depend on objects we do not have but we know about after
> we peek submodule odb" route [*1*], write_sha1_file() needs to be
> told about has_sha1_file_proper(). We may "git add" a new blob in
> the superproject, the blob may not yet exist in *our* repository,
> but may happen to already exist in the submodue odb. In such a
> case, write_sha1_file() has to write that blob in our repository,
> without the existing has_sha1_file() check bypassing it. Otherwise
> our attempt to create a tree that contains that blob will fail,
> saying that the blob only seems to exist to us via submodule odb but
> not in our repository.
Another thing needs to be done for this to work. The current reading
order is packs first, loose objects next. If we create a local loose
duplicate of an alternate packed object, our local version will never
be read. Regardless the submodule odb issue, I think we should prefer
reading local loose objects over alternate packed ones.
--
Duy
^ permalink raw reply
* Re: [PATCH v3 00/11] unify appending of sob
From: Junio C Hamano @ 2013-01-28 5:49 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Brandon Casey, git, pclouds
In-Reply-To: <20130128034831.GQ8206@elie.Belkin>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Brandon Casey wrote:
>
>> Round 3.
>
> Thanks for a pleasant read. My only remaining observations are
> cosmetic, except for a portability question in Duy's test script, a
> small behavior change when the commit message ends with an
> RFC2822-style header with no trailing newline and the possibility of
> tightening the pattern in sequencer.c to match the strictness of
> format-patch (which could easily wait for a later patch).
>
> Jonathan
Thanks for a quick review. I agree that this series is getting very
close with your help.
^ permalink raw reply
* Re: [PATCH v3 09/11] t4014: more tests about appending s-o-b lines
From: Junio C Hamano @ 2013-01-28 5:51 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Brandon Casey, git, pclouds, Brandon Casey
In-Reply-To: <7vboc9zwv1.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>> Is "grep -n" portable? I didn't find any uses of it elsewhere in the
>> testsuite.
>
> Yes, "-n" is in POSIX. Even though we use it ourselves, "git grep"
> supports it, too.
Ehh even though we *DONT* use it ourselves, ... that is.
I do not think we mind, if its use helps our test.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox