* Re: [PATCH v2 06/11] i18n: add--interactive: i18n of help_patch_cmd
From: Junio C Hamano @ 2016-09-25 23:11 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
David Aguilar
In-Reply-To: <1472646690-9699-7-git-send-email-vascomalmeida@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> sub help_patch_cmd {
> - my $verb = lc $patch_mode_flavour{VERB};
> - my $target = $patch_mode_flavour{TARGET};
> - print colored $help_color, <<EOF ;
> -y - $verb this hunk$target
> -n - do not $verb this hunk$target
> -q - quit; do not $verb this hunk or any of the remaining ones
> -a - $verb this hunk and all later hunks in the file
> -d - do not $verb this hunk or any of the later hunks in the file
> -g - select a hunk to go to
> + if ($patch_mode eq 'stage') {
> + print colored $help_color, __(
> +"y - stage this hunk
> +n - do not stage this hunk
> +q - quit; do not stage this hunk or any of the remaining ones
> +a - stage this hunk and all later hunks in the file
> +d - do not stage this hunk or any of the later hunks in the file");
> + } elsif ($patch_mode eq 'stash') {
> + ...
> +"\n";
> }
The same "shouldn't this be done with a table not code?" comment
applies to this as well, though this is not as bad as the previous
one.
^ permalink raw reply
* Re: [PATCH v2 05/11] i18n: add--interactive: mark message for translation
From: Junio C Hamano @ 2016-09-25 23:09 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
David Aguilar
In-Reply-To: <1472646690-9699-6-git-send-email-vascomalmeida@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> - print colored $prompt_color, $patch_mode_flavour{VERB},
> - ($hunk[$ix]{TYPE} eq 'mode' ? ' mode change' :
> - $hunk[$ix]{TYPE} eq 'deletion' ? ' deletion' :
> - ' this hunk'),
> - $patch_mode_flavour{TARGET},
> - " [y,n,q,a,d,/$other,?]? ";
I hate to say this but expanding this single-liner into if/elsif/
cascade of uncountable number of arms is simply a disaster.
> + if ($patch_mode eq 'stage') {
> + if ($hunk[$ix]{TYPE} eq 'mode') {
> + print colored $prompt_color,
> + sprintf(__("Stage mode change [y,n,q,a,d,/%s,?]? "), $other);
> + } elsif ($hunk[$ix]{TYPE} eq 'deletion') {
> + print colored $prompt_color,
> + sprintf(__("Stage deletion [y,n,q,a,d,/%s,?]? "), $other);
> + } else {
> + print colored $prompt_color,
> + sprintf(__("Stage this hunk [y,n,q,a,d,/%s,?]? "), $other);
> + }
> + } elsif ($patch_mode eq 'stash') {
> + ...
> + }
> + }
I wonder if you can make a simple helper function so that the caller
here can still be a single-liner:
print_colored $prompt_color,
sprintf(patch_update_prompt_string($patch_mode, $hunk[$ix]{TYPE}), $other);
where the patch_update_prompt_string helper function would look up
these messages from a table that is looked up by patch-mode and TYPE
and the run __() on it, or something?
^ permalink raw reply
* Re: [PATCH v2 03/11] i18n: add--interactive: mark strings with interpolation for translation
From: Junio C Hamano @ 2016-09-25 22:57 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
David Aguilar
In-Reply-To: <1472646690-9699-4-git-send-email-vascomalmeida@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> @@ -1048,7 +1048,7 @@ sub edit_hunk_manually {
> my $hunkfile = $repo->repo_path . "/addp-hunk-edit.diff";
> my $fh;
> open $fh, '>', $hunkfile
> - or die "failed to open hunk edit file for writing: " . $!;
> + or die sprintf(__("failed to open hunk edit file for writing: %s"), $!);
OK, $! presumably is given in the user's language, so we let
translators prepare the error-specific text and interpolate $! into
it.
Makes sense.
^ permalink raw reply
* Re: [PATCH v2 02/11] i18n: add--interactive: mark simple here documents for translation
From: Junio C Hamano @ 2016-09-25 22:54 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
David Aguilar
In-Reply-To: <1472646690-9699-3-git-send-email-vascomalmeida@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> -Prompt help:
> + print colored $help_color, __(
> +"Prompt help:
> 1 - select a numbered item
> foo - select item based on unique prefix
> - - (empty) select nothing
> -EOF
> + - (empty) select nothing"),
> +"\n";
> }
>
> sub prompt_help_cmd {
> - print colored $help_color, <<\EOF ;
> -Prompt help:
> + print colored $help_color, __(
> +"Prompt help:
> 1 - select a single item
> 3-5 - select a range of items
> 2-3,6-9 - select multiple ranges
> foo - select item based on unique prefix
> -... - unselect specified items
> * - choose all items
> - - (empty) finish selecting
> -EOF
> + - (empty) finish selecting"),
> +"\n";
> }
>
> sub status_cmd {
> @@ -1573,14 +1573,14 @@ sub quit_cmd {
> }
>
> sub help_cmd {
> - print colored $help_color, <<\EOF ;
> -status - show paths with changes
> + print colored $help_color, __(
> +"status - show paths with changes
> update - add working tree state to the staged set of changes
> revert - revert staged set of changes back to the HEAD version
> patch - pick hunks and update selectively
> diff - view diff between HEAD and index
> -add untracked - add contents of untracked files to the staged set of changes
> -EOF
> +add untracked - add contents of untracked files to the staged set of changes"),
> +"\n";
> }
Do we need TRANSLATORS: comment to all of the above not to touch the
command words that are explained and translate only the explanation?
^ permalink raw reply
* Re: [PATCH v2 01/11] i18n: add--interactive: mark strings for translation
From: Junio C Hamano @ 2016-09-25 22:52 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
David Aguilar
In-Reply-To: <1472646690-9699-2-git-send-email-vascomalmeida@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> Mark simple strings (without interpolation) for translation.
>
> Brackets around first parameter of ternary operator is necessary because
> otherwise xgettext fails to extract strings marked for translation from
> the rest of the file.
>
> Signed-off-by: Vasco Almeida <vascomalmeida@sapo.pt>
> ---
> git-add--interactive.perl | 68 +++++++++++++++++++++++++----------------------
> 1 file changed, 36 insertions(+), 32 deletions(-)
>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 822f857..fb8e5de 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -4,6 +4,7 @@ use 5.008;
> use strict;
> use warnings;
> use Git;
> +use Git::I18N;
>
> binmode(STDOUT, ":raw");
>
> @@ -252,7 +253,7 @@ sub list_untracked {
> }
>
> my $status_fmt = '%12s %12s %s';
> -my $status_head = sprintf($status_fmt, 'staged', 'unstaged', 'path');
> +my $status_head = sprintf($status_fmt, __('staged'), __('unstaged'), __('path'));
Wouldn't it make sense to allow translators to tweak $status_fmt if
you are allowing the earlier elements that are formatted with %12s,
as their translation may not fit within that width, in which case
they may want to make these columns wider?
> prompt_yesno(
> - 'Your edited hunk does not apply. Edit again '
> - . '(saying "no" discards!) [y/n]? '
> + # TRANSLATORS: do not translate [y/n]
> + # The program will only accept that input
> + # at this point.
> + __('Your edited hunk does not apply. Edit again '
> + . '(saying "no" discards!) [y/n]? ')
Not just [y/n], but "no" in "saying no discards!" also needs to
stay, no? I wonder if it is a good idea to lose the TRANSLATORS
comment by ejecting "[y/n]" outside the "__()" construct here.
^ permalink raw reply
* Re: [PATCH v2 2/2] mailinfo: unescape quoted-pair in header fields
From: Jakub Narębski @ 2016-09-25 22:38 UTC (permalink / raw)
To: Kevin Daudt, Jeff King; +Cc: Junio C Hamano, git, Swift Geek
In-Reply-To: <20160925201713.GA6937@ikke.info>
W dniu 25.09.2016 o 22:17, Kevin Daudt pisze:
> On Fri, Sep 23, 2016 at 12:15:41AM -0400, Jeff King wrote:
>> Oops, yes. It is beginning to make the "strbuf_swap()" look less
>> convoluted. :)
>>
>
> I've switched to strbuf_swap now, much better. I've implemented
> recursive parsing without looking at what you provided, just to see what
> I'd came up with. Though I've not implemented a recursive descent
> parser, but it might suffice.
I think you can implement a parser handling proper nesting of parens
without recursion.
Though... what is the definition in the RFC?
--
Jakub Narębski
^ permalink raw reply
* [PATCH v3 2/2] mailinfo: unescape quoted-pair in header fields
From: Kevin Daudt @ 2016-09-25 21:08 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Swift Geek, Jeff King, Kevin Daudt
In-Reply-To: <20160925210808.26424-1-me@ikke.info>
rfc2822 has provisions for quoted strings and comments in structured header
fields, but also allows for escaping these with so-called quoted-pairs.
The only thing git currently does is removing exterior quotes, but
quotes within are left alone.
Remove exterior quotes and remove escape characters so that they don't
show up in the author field.
Signed-off-by: Kevin Daudt <me@ikke.info>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Changes since v2:
- handle comments inside comments recursively
- renamed the main function to unquote_quoted_pairs because it also
handles quoted pairs in comments
mailinfo.c | 82 ++++++++++++++++++++++++++++++++++++++++++++
t/t5100-mailinfo.sh | 14 ++++++++
| 5 +++
| 9 +++++
t/t5100/quoted-string.expect | 5 +++
t/t5100/quoted-string.in | 9 +++++
6 files changed, 124 insertions(+)
create mode 100644 t/t5100/comment.expect
create mode 100644 t/t5100/comment.in
create mode 100644 t/t5100/quoted-string.expect
create mode 100644 t/t5100/quoted-string.in
diff --git a/mailinfo.c b/mailinfo.c
index e19abe3..b4118a0 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -54,6 +54,86 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line)
get_sane_name(&mi->name, &mi->name, &mi->email);
}
+static const char *unquote_comment(struct strbuf *outbuf, const char *in)
+{
+ int c;
+ int take_next_litterally = 0;
+
+ strbuf_addch(outbuf, '(');
+
+ while ((c = *in++) != 0) {
+ if (take_next_litterally == 1) {
+ take_next_litterally = 0;
+ } else {
+ switch (c) {
+ case '\\':
+ take_next_litterally = 1;
+ continue;
+ case '(':
+ in = unquote_comment(outbuf, in);
+ continue;
+ case ')':
+ strbuf_addch(outbuf, ')');
+ return in;
+ }
+ }
+
+ strbuf_addch(outbuf, c);
+ }
+
+ return in;
+}
+
+static const char *unquote_quoted_string(struct strbuf *outbuf, const char *in)
+{
+ int c;
+ int take_next_litterally = 0;
+
+ while ((c = *in++) != 0) {
+ if (take_next_litterally == 1) {
+ take_next_litterally = 0;
+ } else {
+ switch (c) {
+ case '\\':
+ take_next_litterally = 1;
+ continue;
+ case '"':
+ return in;
+ }
+ }
+
+ strbuf_addch(outbuf, c);
+ }
+
+ return in;
+}
+
+static void unquote_quoted_pair(struct strbuf *line)
+{
+ struct strbuf outbuf;
+ const char *in = line->buf;
+ int c;
+
+ strbuf_init(&outbuf, line->len);
+
+ while ((c = *in++) != 0) {
+ switch (c) {
+ case '"':
+ in = unquote_quoted_string(&outbuf, in);
+ continue;
+ case '(':
+ in = unquote_comment(&outbuf, in);
+ continue;
+ }
+
+ strbuf_addch(&outbuf, c);
+ }
+
+ strbuf_swap(&outbuf, line);
+ strbuf_release(&outbuf);
+
+}
+
static void handle_from(struct mailinfo *mi, const struct strbuf *from)
{
char *at;
@@ -63,6 +143,8 @@ static void handle_from(struct mailinfo *mi, const struct strbuf *from)
strbuf_init(&f, from->len);
strbuf_addbuf(&f, from);
+ unquote_quoted_pair(&f);
+
at = strchr(f.buf, '@');
if (!at) {
parse_bogus_from(mi, from);
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index c4ed0f4..3e983c0 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -144,4 +144,18 @@ test_expect_success 'mailinfo unescapes with --mboxrd' '
test_cmp expect mboxrd/msg
'
+test_expect_success 'mailinfo handles rfc2822 quoted-string' '
+ mkdir quoted-string &&
+ git mailinfo /dev/null /dev/null <"$DATA"/quoted-string.in \
+ >quoted-string/info &&
+ test_cmp "$DATA"/quoted-string.expect quoted-string/info
+'
+
+test_expect_success 'mailinfo handles rfc2822 comment' '
+ mkdir comment &&
+ git mailinfo /dev/null /dev/null <"$DATA"/comment.in \
+ >comment/info &&
+ test_cmp "$DATA"/comment.expect comment/info
+'
+
test_done
--git a/t/t5100/comment.expect b/t/t5100/comment.expect
new file mode 100644
index 0000000..7228177
--- /dev/null
+++ b/t/t5100/comment.expect
@@ -0,0 +1,5 @@
+Author: A U Thor (this is (really) a comment (honestly))
+Email: somebody@example.com
+Subject: testing comments
+Date: Sun, 25 May 2008 00:38:18 -0700
+
--git a/t/t5100/comment.in b/t/t5100/comment.in
new file mode 100644
index 0000000..c53a192
--- /dev/null
+++ b/t/t5100/comment.in
@@ -0,0 +1,9 @@
+From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001
+From: "A U Thor" <somebody@example.com> (this is \(really\) a comment (honestly))
+Date: Sun, 25 May 2008 00:38:18 -0700
+Subject: [PATCH] testing comments
+
+
+
+---
+patch
diff --git a/t/t5100/quoted-string.expect b/t/t5100/quoted-string.expect
new file mode 100644
index 0000000..cab1bce
--- /dev/null
+++ b/t/t5100/quoted-string.expect
@@ -0,0 +1,5 @@
+Author: Author "The Author" Name
+Email: somebody@example.com
+Subject: testing quoted-pair
+Date: Sun, 25 May 2008 00:38:18 -0700
+
diff --git a/t/t5100/quoted-string.in b/t/t5100/quoted-string.in
new file mode 100644
index 0000000..e2e627a
--- /dev/null
+++ b/t/t5100/quoted-string.in
@@ -0,0 +1,9 @@
+From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001
+From: "Author \"The Author\" Name" <somebody@example.com>
+Date: Sun, 25 May 2008 00:38:18 -0700
+Subject: [PATCH] testing quoted-pair
+
+
+
+---
+patch
--
2.10.0.89.ge802c3a.dirty
^ permalink raw reply related
* [PATCH v3 1/2] t5100-mailinfo: replace common path prefix with variable
From: Kevin Daudt @ 2016-09-25 21:08 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Swift Geek, Jeff King, Kevin Daudt
In-Reply-To: <20160919185440.18234-1-me@ikke.info>
Many tests need to store data in a file, and repeat the same pattern to
refer to that path:
"$TEST_DIRECTORY"/t5100/
Create a variable that contains this path, and use that instead.
Signed-off-by: Kevin Daudt <me@ikke.info>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Changes since v2:
- changed $DATA to $data to indicate it's a script-local variable
t/t5100-mailinfo.sh | 56 +++++++++++++++++++++++++++--------------------------
1 file changed, 29 insertions(+), 27 deletions(-)
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 1a5a546..c4ed0f4 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -7,8 +7,10 @@ test_description='git mailinfo and git mailsplit test'
. ./test-lib.sh
+data="$TEST_DIRECTORY/t5100"
+
test_expect_success 'split sample box' \
- 'git mailsplit -o. "$TEST_DIRECTORY"/t5100/sample.mbox >last &&
+ 'git mailsplit -o. "$data"/sample.mbox >last &&
last=$(cat last) &&
echo total is $last &&
test $(cat last) = 17'
@@ -17,9 +19,9 @@ check_mailinfo () {
mail=$1 opt=$2
mo="$mail$opt"
git mailinfo -u $opt msg$mo patch$mo <$mail >info$mo &&
- test_cmp "$TEST_DIRECTORY"/t5100/msg$mo msg$mo &&
- test_cmp "$TEST_DIRECTORY"/t5100/patch$mo patch$mo &&
- test_cmp "$TEST_DIRECTORY"/t5100/info$mo info$mo
+ test_cmp "$data"/msg$mo msg$mo &&
+ test_cmp "$data"/patch$mo patch$mo &&
+ test_cmp "$data"/info$mo info$mo
}
@@ -27,15 +29,15 @@ for mail in 00*
do
test_expect_success "mailinfo $mail" '
check_mailinfo $mail "" &&
- if test -f "$TEST_DIRECTORY"/t5100/msg$mail--scissors
+ if test -f "$data"/msg$mail--scissors
then
check_mailinfo $mail --scissors
fi &&
- if test -f "$TEST_DIRECTORY"/t5100/msg$mail--no-inbody-headers
+ if test -f "$data"/msg$mail--no-inbody-headers
then
check_mailinfo $mail --no-inbody-headers
fi &&
- if test -f "$TEST_DIRECTORY"/t5100/msg$mail--message-id
+ if test -f "$data"/msg$mail--message-id
then
check_mailinfo $mail --message-id
fi
@@ -45,7 +47,7 @@ done
test_expect_success 'split box with rfc2047 samples' \
'mkdir rfc2047 &&
- git mailsplit -orfc2047 "$TEST_DIRECTORY"/t5100/rfc2047-samples.mbox \
+ git mailsplit -orfc2047 "$data"/rfc2047-samples.mbox \
>rfc2047/last &&
last=$(cat rfc2047/last) &&
echo total is $last &&
@@ -56,18 +58,18 @@ do
test_expect_success "mailinfo $mail" '
git mailinfo -u $mail-msg $mail-patch <$mail >$mail-info &&
echo msg &&
- test_cmp "$TEST_DIRECTORY"/t5100/empty $mail-msg &&
+ test_cmp "$data"/empty $mail-msg &&
echo patch &&
- test_cmp "$TEST_DIRECTORY"/t5100/empty $mail-patch &&
+ test_cmp "$data"/empty $mail-patch &&
echo info &&
- test_cmp "$TEST_DIRECTORY"/t5100/rfc2047-info-$(basename $mail) $mail-info
+ test_cmp "$data"/rfc2047-info-$(basename $mail) $mail-info
'
done
test_expect_success 'respect NULs' '
- git mailsplit -d3 -o. "$TEST_DIRECTORY"/t5100/nul-plain &&
- test_cmp "$TEST_DIRECTORY"/t5100/nul-plain 001 &&
+ git mailsplit -d3 -o. "$data"/nul-plain &&
+ test_cmp "$data"/nul-plain 001 &&
(cat 001 | git mailinfo msg patch) &&
test_line_count = 4 patch
@@ -75,52 +77,52 @@ test_expect_success 'respect NULs' '
test_expect_success 'Preserve NULs out of MIME encoded message' '
- git mailsplit -d5 -o. "$TEST_DIRECTORY"/t5100/nul-b64.in &&
- test_cmp "$TEST_DIRECTORY"/t5100/nul-b64.in 00001 &&
+ git mailsplit -d5 -o. "$data"/nul-b64.in &&
+ test_cmp "$data"/nul-b64.in 00001 &&
git mailinfo msg patch <00001 &&
- test_cmp "$TEST_DIRECTORY"/t5100/nul-b64.expect patch
+ test_cmp "$data"/nul-b64.expect patch
'
test_expect_success 'mailinfo on from header without name works' '
mkdir info-from &&
- git mailsplit -oinfo-from "$TEST_DIRECTORY"/t5100/info-from.in &&
- test_cmp "$TEST_DIRECTORY"/t5100/info-from.in info-from/0001 &&
+ git mailsplit -oinfo-from "$data"/info-from.in &&
+ test_cmp "$data"/info-from.in info-from/0001 &&
git mailinfo info-from/msg info-from/patch \
<info-from/0001 >info-from/out &&
- test_cmp "$TEST_DIRECTORY"/t5100/info-from.expect info-from/out
+ test_cmp "$data"/info-from.expect info-from/out
'
test_expect_success 'mailinfo finds headers after embedded From line' '
mkdir embed-from &&
- git mailsplit -oembed-from "$TEST_DIRECTORY"/t5100/embed-from.in &&
- test_cmp "$TEST_DIRECTORY"/t5100/embed-from.in embed-from/0001 &&
+ git mailsplit -oembed-from "$data"/embed-from.in &&
+ test_cmp "$data"/embed-from.in embed-from/0001 &&
git mailinfo embed-from/msg embed-from/patch \
<embed-from/0001 >embed-from/out &&
- test_cmp "$TEST_DIRECTORY"/t5100/embed-from.expect embed-from/out
+ test_cmp "$data"/embed-from.expect embed-from/out
'
test_expect_success 'mailinfo on message with quoted >From' '
mkdir quoted-from &&
- git mailsplit -oquoted-from "$TEST_DIRECTORY"/t5100/quoted-from.in &&
- test_cmp "$TEST_DIRECTORY"/t5100/quoted-from.in quoted-from/0001 &&
+ git mailsplit -oquoted-from "$data"/quoted-from.in &&
+ test_cmp "$data"/quoted-from.in quoted-from/0001 &&
git mailinfo quoted-from/msg quoted-from/patch \
<quoted-from/0001 >quoted-from/out &&
- test_cmp "$TEST_DIRECTORY"/t5100/quoted-from.expect quoted-from/msg
+ test_cmp "$data"/quoted-from.expect quoted-from/msg
'
test_expect_success 'mailinfo unescapes with --mboxrd' '
mkdir mboxrd &&
git mailsplit -omboxrd --mboxrd \
- "$TEST_DIRECTORY"/t5100/sample.mboxrd >last &&
+ "$data"/sample.mboxrd >last &&
test x"$(cat last)" = x2 &&
for i in 0001 0002
do
git mailinfo mboxrd/msg mboxrd/patch \
<mboxrd/$i >mboxrd/out &&
- test_cmp "$TEST_DIRECTORY"/t5100/${i}mboxrd mboxrd/msg
+ test_cmp "$data"/${i}mboxrd mboxrd/msg
done &&
sp=" " &&
echo "From " >expect &&
--
2.10.0.89.ge802c3a.dirty
^ permalink raw reply related
* Re: [PATCH v2 2/2] mailinfo: unescape quoted-pair in header fields
From: Kevin Daudt @ 2016-09-25 20:17 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Swift Geek
In-Reply-To: <20160923041540.5fvl6ytp2tvcflsk@sigill.intra.peff.net>
On Fri, Sep 23, 2016 at 12:15:41AM -0400, Jeff King wrote:
> On Thu, Sep 22, 2016 at 03:17:23PM -0700, Junio C Hamano wrote:
>
> > Jeff King <peff@peff.net> writes:
> >
> > > On Mon, Sep 19, 2016 at 08:54:40PM +0200, Kevin Daudt wrote:
> > >
> > >> + ...
> > >> + while ((c = *in++) != 0) {
> > >> + if (take_next_literally) {
> > >> + take_next_literally = 0;
> > >> + } else {
> > >> [...]
> > >> + }
> > >> +
> > >> + strbuf_addch(line, c);
> > >> + }
> > >> +}
> > >
> > > It needs to `free(in)` at the end of the function.
> >
> > Ehh, in has been incremented and is pointing at the terminating NUL
> > there, so it would be more like
> >
> > char *to_free, *in;
> >
> > to_free = strbuf_detach(line, NULL);
> > in = to_free;
> > ...
> > while ((c = *in++)) {
> > ...
> > }
> > free(to_free);
> >
> > I would think ;-).
>
> Oops, yes. It is beginning to make the "strbuf_swap()" look less
> convoluted. :)
>
I've switched to strbuf_swap now, much better. I've implemented
recursive parsing without looking at what you provided, just to see what
I'd came up with. Though I've not implemented a recursive descent
parser, but it might suffice.
I'm sending the patches now.
^ permalink raw reply
* [RFC PATCH v3] revision: new rev^-n shorthand for rev^n..rev
From: Vegard Nossum @ 2016-09-25 19:13 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Santi Béjar, Kevin Bracey, Philip Oakley,
Matthieu Moy, Ramsay Jones, Jakub Narębski, Vegard Nossum
I often use rev^..rev to get all the commits in the branch that was merged
in by the merge commit 'rev' (including the merge itself). To save typing
(or copy-pasting, if the rev is long -- like a full SHA-1 or branch name)
we can make rev^- a shorthand for that.
The existing syntax rev^! seems like it should do the same thing, but it
doesn't really do the right thing for merge commits (it doesn't include
the commits from side branches).
As a natural generalisation, we also accept rev^-n where n excludes the
nth parent of rev. For example, for a two-parent merge, you can use rev^-2
to get the set of commits which were made to the main branch while the
topic branch was prepared.
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
[v2: Use ^- instead of % as suggested by Junio Hamano and use some
common helper functions for parsing.]
[v3: Use 'struct object_id' instead of 'char[20]' and add some tests as
suggested by Matthieu Moy; fix missing '-' in Documentation/revisions.txt
as suggested by Ramsay Jones; misc changelog + documentation fixes as
suggested by Philip Oakley.]
Thanks for the helpful comments so far! I tried to take everything into
account. It's my first git.git patch so I'm not too familiar with how the
tests are set up. Should I add more tests than this?
---
Documentation/revisions.txt | 15 +++++++
builtin/rev-parse.c | 28 +++++++++++++
revision.c | 91 +++++++++++++++++++++++++++++++++++++++++
revision.h | 1 +
t/t6070-rev-parent-exclusion.sh | 38 +++++++++++++++++
5 files changed, 173 insertions(+)
create mode 100755 t/t6070-rev-parent-exclusion.sh
diff --git Documentation/revisions.txt Documentation/revisions.txt
index 4bed5b1..8a27964 100644
--- Documentation/revisions.txt
+++ Documentation/revisions.txt
@@ -281,6 +281,15 @@ is a shorthand for 'HEAD..origin' and asks "What did the origin do since
I forked from them?" Note that '..' would mean 'HEAD..HEAD' which is an
empty range that is both reachable and unreachable from HEAD.
+Parent Exclusion Notation
+~~~~~~~~~~~~~~~~~~~~~~~~~
+The '<rev>{caret}-{<n>}', Parent Exclusion Notation::
+Shorthand for '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
+given. This is typically useful for merge commits where you
+can just pass '<commit>{caret}-' to get all the commits in the branch
+that was merged in merge commit '<commit>' (including '<commit>'
+itself).
+
Other <rev>{caret} Parent Shorthand Notations
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Two other shorthands exist, particularly useful for merge commits,
@@ -316,6 +325,10 @@ Revision Range Summary
<rev2> but exclude those that are reachable from both. When
either <rev1> or <rev2> is omitted, it defaults to `HEAD`.
+'<rev>{caret}-{<n>}', e.g. 'HEAD{caret}-, HEAD{caret}-2'::
+ Equivalent to '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
+ given.
+
'<rev>{caret}@', e.g. 'HEAD{caret}@'::
A suffix '{caret}' followed by an at sign is the same as listing
all parents of '<rev>' (meaning, include anything reachable from
@@ -339,6 +352,8 @@ spelt out:
C I J F C
B..C = ^B C C
B...C = B ^F C G H D E B C
+ B^- = B^..B
+ = B ^B^1 E I J F B
C^@ = C^1
= F I J F
B^@ = B^1 B^2 B^3
diff --git builtin/rev-parse.c builtin/rev-parse.c
index 76cf05e..2c573f1 100644
--- builtin/rev-parse.c
+++ builtin/rev-parse.c
@@ -292,6 +292,32 @@ static int try_difference(const char *arg)
return 0;
}
+static int try_parent_exclusion(const char *arg)
+{
+ int ret = 0;
+ char *to_rev = NULL;
+ char *from_rev = NULL;
+ struct object_id to_oid;
+ struct object_id from_oid;
+
+ if (parse_parent_exclusion(arg, &to_rev, &from_rev))
+ goto out;
+ if (get_oid(to_rev, &to_oid))
+ goto out;
+ if (get_oid(from_rev, &from_oid))
+ goto out;
+
+ show_rev(NORMAL, to_oid.hash, to_rev);
+ show_rev(REVERSED, from_oid.hash, from_rev);
+
+ ret = 1;
+
+out:
+ free(to_rev);
+ free(from_rev);
+ return ret;
+}
+
static int try_parent_shorthands(const char *arg)
{
char *dotdot;
@@ -839,6 +865,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
/* Not a flag argument */
if (try_difference(arg))
continue;
+ if (try_parent_exclusion(arg))
+ continue;
if (try_parent_shorthands(arg))
continue;
name = arg;
diff --git revision.c revision.c
index 969b3d1..602bf88 100644
--- revision.c
+++ revision.c
@@ -1419,6 +1419,93 @@ static void prepare_show_merge(struct rev_info *revs)
revs->limited = 1;
}
+/*
+ * If 'arg' is on the form '<rev>^-{<n>}', then return 0 and
+ * '*to_rev' and '*from_rev' will contain '<rev>' and '<rev>^<n>',
+ * respectively.
+ */
+int parse_parent_exclusion(const char *arg, char **to_rev, char **from_rev)
+{
+ char *caret;
+ unsigned int n = 1;
+
+ /*
+ * <rev>^-{<n>} is shorthand for <rev>^<n>..<rev>, with <n> = 1 if
+ * not given. This is typically used for merge commits where you
+ * can just pass '<merge>^-' and it will show you all the commits in
+ * the branch that was merged.
+ */
+
+ if (!(caret = strstr(arg, "^-")))
+ return 1;
+ if (caret[2]) {
+ char *end;
+ n = strtoul(&caret[2], &end, 10);
+ if (*end != '\0')
+ return 1;
+ }
+ *to_rev = xstrndup(arg, caret - arg);
+ *from_rev = xstrfmt("%s^%u", *to_rev, n);
+ return 0;
+}
+
+static int handle_parent_exclusion(const char *arg, struct rev_info *revs, int flags)
+{
+ int ret = 1;
+ char *to_rev = NULL;
+ char *from_rev = NULL;
+ struct object_id to_oid;
+ struct object_id from_oid;
+
+ struct object *a_obj, *b_obj;
+ unsigned int flags_exclude = flags ^ (UNINTERESTING | BOTTOM);
+ unsigned int a_flags;
+
+ /*
+ * <rev>^-{<n>} is shorthand for <rev>^<n>..<rev>, with <n> = 1 if
+ * not given. This is typically used for merge commits where you
+ * can just pass <merge>^- and it will show you all the commits in
+ * the branches that were merged.
+ */
+
+ if (parse_parent_exclusion(arg, &to_rev, &from_rev))
+ goto out;
+
+ if (get_oid(to_rev, &to_oid)) {
+ if (revs->ignore_missing)
+ goto out;
+ die("Unknown revision %s", to_rev);
+ }
+
+ if (get_oid(from_rev, &from_oid)) {
+ if (revs->ignore_missing)
+ goto out;
+ die("Unknown revision %s", from_rev);
+ }
+
+ a_obj = parse_object(from_oid.hash);
+ b_obj = parse_object(to_oid.hash);
+ if (!a_obj || !b_obj) {
+ if (revs->ignore_missing)
+ goto out;
+ die("Invalid revision range %s", arg);
+ }
+
+ a_flags = flags_exclude;
+ a_obj->flags |= a_flags;
+ b_obj->flags |= flags;
+ add_rev_cmdline(revs, a_obj, from_rev, REV_CMD_LEFT, a_flags);
+ add_pending_object(revs, a_obj, from_rev);
+ add_rev_cmdline(revs, b_obj, to_rev, REV_CMD_RIGHT, flags);
+ add_pending_object(revs, b_obj, to_rev);
+
+ ret = 0;
+out:
+ free(to_rev);
+ free(from_rev);
+ return ret;
+}
+
int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsigned revarg_opt)
{
struct object_context oc;
@@ -1519,6 +1606,10 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
}
*dotdot = '.';
}
+
+ if (!handle_parent_exclusion(arg, revs, flags))
+ return 0;
+
dotdot = strstr(arg, "^@");
if (dotdot && !dotdot[2]) {
*dotdot = 0;
diff --git revision.h revision.h
index 9fac1a6..ca5bebc 100644
--- revision.h
+++ revision.h
@@ -243,6 +243,7 @@ extern int setup_revisions(int argc, const char **argv, struct rev_info *revs,
extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
const struct option *options,
const char * const usagestr[]);
+extern int parse_parent_exclusion(const char *arg, char **to_rev, char **from_rev);
#define REVARG_CANNOT_BE_FILENAME 01
#define REVARG_COMMITTISH 02
extern int handle_revision_arg(const char *arg, struct rev_info *revs,
diff --git t/t6070-rev-parent-exclusion.sh t/t6070-rev-parent-exclusion.sh
new file mode 100755
index 0000000..e02da47
--- /dev/null
+++ t/t6070-rev-parent-exclusion.sh
@@ -0,0 +1,38 @@
+#!/bin/sh
+
+test_description='rev-list/rev-parse rev^- parsing'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+ test_commit one &&
+ test_commit two &&
+ test_commit three &&
+
+ # Merge in a branch for testing ^-
+ git checkout -b branch &&
+ git checkout HEAD^^ &&
+ git merge -m merge --no-edit --no-ff branch &&
+ git checkout -b merge
+'
+
+# The merged branch has 2 commits + the merge
+test_expect_success 'rev-list --count merge^- = merge^..merge' '
+ git rev-list --count merge^..merge >expect &&
+ echo 3 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'rev-parse merge^- = merge^..merge' '
+ git rev-parse merge^..merge >expect &&
+ git rev-parse merge^- >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'rev-list merge^- = merge^..merge' '
+ git rev-list merge^..merge >expect &&
+ git rev-list merge^- >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
2.10.0.rc0.1.g07c9292
^ permalink raw reply related
* Re: [PATCH] git-gui: stop using deprecated merge syntax
From: Junio C Hamano @ 2016-09-25 18:39 UTC (permalink / raw)
To: Johannes Sixt; +Cc: René Scharfe, Git List, Pat Thoyts, Dennis Kaarsemaker
In-Reply-To: <a5b001fd-3ba2-bcc0-2104-eb630796ab09@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 24.09.2016 um 13:30 schrieb René Scharfe:
>> Starting with v2.5.0 git merge can handle FETCH_HEAD internally and
>> warns when it's called like 'git merge <message> HEAD <commit>' because
>> that syntax is deprecated. Use this feature in git-gui and get rid of
>> that warning.
>>
>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>> ---
>> Tested only _very_ lightly!
>>
>> git-gui/lib/merge.tcl | 7 +------
>> 1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/git-gui/lib/merge.tcl b/git-gui/lib/merge.tcl
>> index 460d32f..5ab6f8f 100644
>> --- a/git-gui/lib/merge.tcl
>> +++ b/git-gui/lib/merge.tcl
>> @@ -112,12 +112,7 @@ method _start {} {
>> close $fh
>> set _last_merged_branch $branch
>>
>> - set cmd [list git]
>> - lappend cmd merge
>> - lappend cmd --strategy=recursive
>> - lappend cmd [git fmt-merge-msg <[gitdir FETCH_HEAD]]
>> - lappend cmd HEAD
>> - lappend cmd $name
>> + set cmd [list git merge --strategy=recursive FETCH_HEAD]
>>
>> ui_status [mc "Merging %s and %s..." $current_branch $stitle]
>> set cons [console::new [mc "Merge"] "merge $stitle"]
>>
>
> Much better than my version. I had left fmt-merge-msg and added
> --no-log to treat merge.log config suitably. But this works too, and
> is much more obvious.
>
> Tested-by: Johannes Sixt <j6t@kdbg.org>
Thanks both.
We can finally deprecate and remove the ancient "git merge" syntax
with this, which should make me happy, but with git-gui maintainer
missing, it nudges me to be an interim maintainer one more time.
Sigh...
^ permalink raw reply
* Re: [PATCH 0/3] recursive support for ls-files
From: Junio C Hamano @ 2016-09-25 18:38 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jeff King, git
In-Reply-To: <20160925163244.GB66080@google.com>
Brandon Williams <bmwill@google.com> writes:
> On 09/25, Jeff King wrote:
>> On Fri, Sep 23, 2016 at 05:13:31PM -0700, Brandon Williams wrote:
>>
>> > After looking at the feedback I rerolled a few things, in particular the
>> > --submodule_prefix option that existed to give a submodule context about where
>> > it had been invoked from. People didn't seem to like the idea of exposing this
>> > to the users (yet anyways) so I removed it as an option and instead have it
>> > being passed to a child process via an environment variable
>> > GIT_INTERNAL_SUBMODULE_PREFIX. This way we don't have to support anything to
>> > external users at the moment.
>>
>> I think we can still have it as a command-line argument and declare it
>> internal. It's not like environment variables cannot also be set by our
>> callers. :)
>>
>> I don't mind it as an environment variable, though. In some ways it
>> makes things easier. I just think "internal versus external" and the
>> exact implementation are orthogonal.
>
> We may still want it to be an option at some point in the future. This
> way we can revisit making it an option once we know more about the other
> uses it could have (aside from just being for submodules as someone
> suggested).
I do not think it makes too much of a difference between environment
and command line option. We need an update to the "git" potty to
say "you told me to use the submodule-prefix feature, but this
subcommand is not prepared to accept it (yet)" and cause it to error
out either way, which would mean that a series that introduces the
feature needs to touch "git.c" anyway, so I would have expected us
to add command line option first, simply because "git.c" is where it
happens, optionally with the support for the environment variable,
not the other way around.
>> > Also fixed a bug (and added a test) for the -z options as pointed out by Jeff
>> > King.
>>
>> Hmm. It is broken after patch 2, and then fixed in patch 3. Usually we'd
>> try not to have a broken state in the history. It's less important in
>> this case, because the breakage is not a regression
>> (--recurse-submodules is a new feature, so you could consider it "not
>> working" until the 3rd patch). But I think it's still a good rule to
>> follow, because it makes the commits easier to review, look at later,
>> etc.
>>
>> For that matter, I do not understand why options like "-s" get enabled
>> in patch 3. I do not mind them starting as disabled in patch 2, but it
>> seems like "pass along some known-safe options" should be its own patch
>> somewhere between patches 2 and 3.
Yes, exactly.
An obvious lazy way out to avoid breakage-in-the-middle and make
incremental progress would be to squash everything into one patch,
but we should and we should be able to do better.
I'd imagine this three-patch series would be more pleasant for
future readers if it were structured like:
[1/3] introduces the submodule-prefix as a global feature; at the
least it needs a way to invoke (either an environment, or an
option to "git" potty, or both) and prevent mistakes by
erroring out when it is attempted to call a subcommand that
does not support the feature (yet).
[2/3] adds the --recurse-submodule feature in a limited form to
"ls-files". I'd suggest for this step to pass through all
options and arguments that are safe and reasonably useful
to pass through without needing anything more than "ah, this
option was given, so let's stuff it to the argv-array". An
attempt to give things that are not yet passed through until
3/3 to lead to an error that says it is not allowed (yet).
[3-N] each of the remaining steps after 3/N adds support for one
more thing to be passed that 2/3 refrained from doing, by
doing more than just "pass it in argv-array", and then remove
the "not yet supported" error that added by 2/3 for that one
thing. The first of these "more things" would be to support
pathspecs as the receiving side would need code changes for
the matching logic. There may be more, or there may be
nothing else that requires 4/N, 5/N, etc.
>> There are some other options that are ignored (neither disabled nor
>> passed along to children). Most of them are related to exclusions, which
>> I _think_ are safe to ignore (they do not do anything interesting unless
>> you specify "-o", which is explicitly disabled).
Sure. I agree that some options fall outside of "safe and reasonably
useful to pass through" criteria and it is OK not to support passing
them through. I however think we should detect mistakes in the
caller, though.
I wonder if this will be common enough in the future that we are
better off adding a bit or two to the parse-options infrastructure.
Thinking out loud, would an enhancement like this be sufficient for
this particular series, and still useful in more general cases?
- allow the caller of parse_options() to mark each entry with a
handful of bits with no meaning to parse-options API. "ls-files"
may use this mechanism to mark options that to be passed through
and options that must be rejected when --recurse-submodules is
asked.
- parse_options() will mark each entry as "this option was given"
while doing its work. Note that "--no-foo" counts as "option
'foo' was given".
With these, after your call to parse_options() returns, you could
notice that recurse-submodules was asked for by inspecting your own
"static int recurse_submodules" global variable, and then iterate
over builtin_ls_files_options[] array yourself to see if an option
that you marked with "must be rejected while recursing" bit using
mechanism #1 was seen by parse_options() by relying on mechanism #2.
You could also add a new helper macro to parse-options API that
iterates over the options[] array, i.e.
for_each_parse_options_array(opt) {
if (!opt_was_used(opt))
continue;
if (opt_custom_bit(opt) & NOT_IN_RECURSIVE)
die("'%s' cannot be used with --recurse-submodules",
parse_options_name(opt))
}
or something like that, but we'd need to gain experience with the
mechanism #1 and #2 first before deciding if such a helper is useful
(iow, I do not think we need it from day one, if we go this route).
^ permalink raw reply
* Re: [PATCH v3 0/2] patch-id for merges
From: Johannes Schindelin @ 2016-09-25 18:25 UTC (permalink / raw)
To: Jeff King
Cc: git, Michael Haggerty, Kevin Willford, Xiaolong Ye, Josh Triplett
In-Reply-To: <20160909203406.5j5pmom442yoe4su@sigill.intra.peff.net>
Hi Peff,
On Fri, 9 Sep 2016, Jeff King wrote:
> Frankly, I still like v2 better, but I do not feel like arguing with
> Johannes about it anymore.
Sorry that I was a pest. I do care deeply about making our code cleaner,
though, which in my mind means less of this "let exit() clean everything
up and give the callers no chance to do anything anymore" approach, which
to me always felt like we're basically teaching Git to shrug instead of
being useful when something bad happens.
Ciao,
Dscho
^ permalink raw reply
* Re: Git garden shears, was Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file
From: Johannes Schindelin @ 2016-09-25 18:16 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Junio C Hamano, git, Stefan Haller
In-Reply-To: <f7fe3f15-664e-61a5-6101-30ed4946c107@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 8819 bytes --]
Hi Kuba,
On Wed, 21 Sep 2016, Jakub Narębski wrote:
> W dniu 11.09.2016 o 10:33, Johannes Schindelin napisał:
> > On Fri, 9 Sep 2016, Jakub Narębski wrote:
> [...]
>
> >> When preserving merges, there are (as far as I understand it), two
> >> problems:
> >> - what it means to preserve changes (which change to pick,
> >> that is what is the mainline changes rebase is re-applying)
> >> - what are parents of the merge commit (at least one parent
> >> would be usually rewritten)
> >>
> >> Maybe the internal (and perhaps also user-visible) representation
> >> of merge in instruction sheet could use the notation of filter-branch,
> >> that is 'map(<sha-1>)'... it could also imply the mainline.
> >>
> >> That is the instruction in the internal instruction sheet could
> >> look like this:
> >>
> >> merge -m 1 map(2fd4e1c6...) da39a3ee... \t Merge 'foo' into master
> >>
> >>
> >> Note that it has nothing to do with this series!
> >
> > Right. But I did solve that already. In the Git garden shears [*1*]
> > (essentially my New And Improved attempt at recreating branch structures
> > while rebasing), I generate and process scripts like this:
> >
> > mark onto
> >
> > # Branch: super-cool-feature
> > rewind onto
> > pick 00001 feature
> > pick 00002 documentation
> > mark super-cool-feature
> >
> > # Branch: typo-fix
> > rewind onto
> > pick 0000a fix a tyop
>
> There probably should be there
>
> mark typo-fix
Correct. Sorry for the omission.
> > rewind onto
> > merge -C cafebabe super-cool-feature
> > merge -C babecafe typo-fix
> >
> > cleanup super-cool-feature typo-fix
> >
> > Of course this will change a little, still, once I get around to implement
> > this on top of the rebase--helper.
>
> Do I understand it correctly that it is user-visible instruction sheet, and
> not the internal instruction sheet for sequencer? This looks very nice
> and is well readable.
It is intended as that.
Currently I need a little trickery to make this work, as rebase -i does
not understand rewind nor merge. The trick is to re-use the shears.sh
script as editor that then populates the edit script, calls the real
editor, and then installs a temporary alias that gets called for all
custom commands via exec (turning e.g. "rewind abc" into "exec git .r
rewind abc").
The temporary alias point back to the shears script, too, of course.
In the end, I hope to teach the sequencer a variant of this dialect, as
well as the trick to generate such edit scripts.
> I guess that it needs to be pre-populated by Git based on topology of the
> branch being rebased.
Yes. The shears.sh script is in charge of that, and it has to perform a
couple of Git calls to do so.
> As I see, there are three basic topologies of non-linear branch to be
> rebased; all else is combination of thereof, or derivative:
>
> 1. Merge commit without branching point, that is we need to go
> from the following situation
>
> *---*---*---#---o---o---o <-- old base
> \\
> \\=a===b===M===c <-- branch being rebased
> /
> ...---x---x---x-/ <-- side branch
>
> to the following:
>
> *---*---*---#---o---o---o
> \
> \-a'--b'--M'--c'
> /
> ...---x---x---x-------------/
In other words: rebasing a merge commit merging non-rebased commits.
This is not yet supported by the shears script, as it would require logic
that is not only slow in shell script, but also convoluted. IOW this
feature waits for the sequencer to know how to run regular rebase -i
already.
> I think this case is the only one supported by `--preserve-merges`,
> but I may be mistaken - I never had the need to use this feature IRL.
No, -p would handle merges of non-rebased commits as well as merges of
to-be-rebased commits.
> 2. Branching point without accompanying merge commit, or in other words
> rebasing many branches tied together; a shrub if you will. That is,
> we want to go from the following situation:
>
> *---*---*---#---o---o---o <-- old base
> \
> \--a---b---c <-- branch being rebased
> \
> \-1 <-- dependent branch
>
> to the following one:
>
> *---*---*---#---o---o---o
> \
> \--a'--b'--c'
> \
> \-1'
This is outside the scope of rebase -i (and of the shears), as you are
talking about *parallel* rebases. I don't do that, nor does rebase -i,
rebase -p nor the shears.
> I don't think Git supports something like that out of the box, but it
> is not hard to create something like that "by hand". It is not much
> of a problem... unless you forget to rebase the second dependent branch.
The safer thing to do, of course, is to merge all those tips back into the
branch you then rebase in one go.
> 3. Branching point with merge point, that is subbranch created and
> merged - an "eye" (it is not a loop in DAG):
>
> *---*---*---#---o---o---o <-- old base
> \
> \--a---b---c---M---d <-- branch being rebased
> \ /
> \-1---2-/ [ <-- possibly a branch ]
>
> All edges are directed edges, with arrows pointing from right to
> left; that is *---* is really *<---*
>
> The expected result is:
>
> *---*---*---#---o---o---o
> \
> \--a'--b'--c'--M'--d'
> \ /
> \-1'--2'/
>
> I guess that is the main purpose of your git-garden-shears script,
> isn't it?
Yes.
> > For example, I am not so hot about the "merge -C ..." syntax. I'll
> > probably split that into a "remerge <SHA-1> <mark>" and a new "merge
> > <mark>" command (the latter asking interactively for the merge commit
> > message).
>
> There is also an additional complication in that merge commit message
> may be *partially* automatically generated. First there is the subject
> generated by 'git merge' ("Merge branch 'foo'") or 'git pull <URL>'.
> It might have been translated, or extended. Second there is a place
> for branch cover letter. Third, subject to merge.log / merge.summary
> there is a shortlog.
True. The current shears will simply use the original commit message
verbatim. And I think it would be wise to keep this behaviour until the
sequencer knows how to recreate merges.
Once that happens, I cordially invite you to implement any merge commit
message munging mode your heart desires, on top of the straight port of
the shears.
> From those shortlog should be surely updated to correspond to the
> post-rebase state. The first line could be used to pre-populate
> mark lines, but during merge it should be, I think, updated to the
> new name of internal branch if it was changed.
Surely. Or not so surely. Too unsurely, for sure, to be discussed without
a dedicated patch series submitted by a dedicated person requiring that
feature.
> As to 'merge -C <sha1> <marker>' vs 'remerge <sha1> <marker>',
> I don't have specified opinion. It would be nice to have one
> character shortcuts for insn sheet instructions, to which
> 'm -C <sha1> <marker>' is more amendable...
I am pretty certain that you will find yourself disagreeing with that
statement once you used the shears for a while.
I use it for over a year now, and I *never* had to type any "merge -C"
command (i.e. the command I would call "remerge"). The reason is that you
simply move those generated lines, and that there is no scenario I
encountered where you will want to insert a merge manually: you simply
merge all you want *before* or *after* rebasing.
As to "merge <marker>": those are quite common, in particular when
splitting patch series into parts.
> > No :1 or some such. That's machine readable. But it's utter nonsense for
> > user-facing UIs.
>
> Of course. It's all right for machine-facing instructions, like the
> 'todo' file for the sequencer, or for git-fast-import stream...
The 'todo' file for the sequencer *should* be human-friendly. There is no
reason not to make it so, in particular when you can simply reuse the refs
machinery for marks, and when it facilitates debugging (and I can testify
to that: it does, oh yeah, it does...).
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v4 2/2] gitweb: use highlight's shebang detection
From: Jakub Narębski @ 2016-09-25 18:04 UTC (permalink / raw)
To: Ian Kelling, git
In-Reply-To: <20160924223258.9449-2-ian@iankelling.org>
W dniu 25.09.2016 o 00:32, Ian Kelling pisze:
> The "highlight" binary can, in some cases, determine the language type
> by the means of file contents, for example the shebang in the first line
> for some scripting languages. Make use of this autodetection for files
> which syntax is not known by gitweb. In that case, pass the blob
> contents to "highlight --force"; the parameter is needed to make it
> always generate HTML output (which includes HTML-escaping).
>
> Although we now run highlight on files which do not end up highlighted,
> performance is virtually unaffected because when we call highlight, it
> is used for escaping HTML. In the case that highlight is used, gitweb
> calls sanitize() instead of esc_html(), and the latter is significantly
> slower (it does more, being roughly a superset of sanitize()). Simple
> benchmark comparing performance of 'blob' view of files without syntax
> highlighting in gitweb before and after this change indicates ±1%
> difference in request time for all file types. Benchmark was performed
> on local instance on Debian, using Apache/2.4.23 web server and CGI.
>
> Document the feature and improve syntax highlight documentation, add
> test to ensure gitweb doesn't crash when language detection is used.
>
> Signed-off-by: Ian Kelling <ian@iankelling.org>
For what it is worth it:
Acked-by: Jakub Narębski <jnareb@gmail.com>
(but unfortunately *not* tested by).
> ---
>
> Notes:
> The only change from v3 is the commit message as suggested by Jakub
> Narębski
>
> Documentation/gitweb.conf.txt | 21 ++++++++++++++-------
> gitweb/gitweb.perl | 10 +++++-----
> t/t9500-gitweb-standalone-no-errors.sh | 8 ++++++++
> 3 files changed, 27 insertions(+), 12 deletions(-)
^ permalink raw reply
* Re: [PATCH v4 1/2] gitweb: remove unused guess_file_syntax() parameter
From: Jakub Narębski @ 2016-09-25 17:57 UTC (permalink / raw)
To: Ian Kelling, git
In-Reply-To: <20160924223258.9449-1-ian@iankelling.org>
W dniu 25.09.2016 o 00:32, Ian Kelling pisze:
> Subject: gitweb: remove unused guess_file_syntax() parameter
>
> Signed-off-by: Ian Kelling <ian@iankelling.org>
Acked-by: Jakub Narębski <jnareb@gmail.com>
> ---
>
> Notes:
> The only change from v3 is a more descriptive commit message
>
> gitweb/gitweb.perl | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 33d701d..6cb4280 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3913,7 +3913,7 @@ sub blob_contenttype {
> # guess file syntax for syntax highlighting; return undef if no highlighting
> # the name of syntax can (in the future) depend on syntax highlighter used
> sub guess_file_syntax {
> - my ($highlight, $mimetype, $file_name) = @_;
> + my ($highlight, $file_name) = @_;
> return undef unless ($highlight && defined $file_name);
> my $basename = basename($file_name, '.in');
> return $highlight_basename{$basename}
> @@ -7062,7 +7062,7 @@ sub git_blob {
> $have_blame &&= ($mimetype =~ m!^text/!);
>
> my $highlight = gitweb_check_feature('highlight');
> - my $syntax = guess_file_syntax($highlight, $mimetype, $file_name);
> + my $syntax = guess_file_syntax($highlight, $file_name);
> $fd = run_highlighter($fd, $highlight, $syntax)
> if $syntax;
>
>
^ permalink raw reply
* Re: [RFC PATCH v2] revision: new rev^-n shorthand for rev^n..rev
From: Philip Oakley @ 2016-09-25 17:37 UTC (permalink / raw)
To: Vegard Nossum, git
Cc: Junio C Hamano, Santi Béjar, Kevin Bracey, Vegard Nossum
In-Reply-To: <20160925085511.12515-1-vegard.nossum@oracle.com>
From: "Vegard Nossum" <vegard.nossum@oracle.com>
>I use rev^..rev daily, and I'm surely not the only one.
Not everyone knows the 'trick' and may not use it daily.
Consider stating what it is useful for (e.g. "useful to get the commits and
all commits in the branches that were merged into commit" - paraphrased
from the doc text)
> To save typing
> (or copy-pasting, if the rev is long -- like a full SHA-1 or branch name)
> we can make rev^- a shorthand for that.
>
> The existing syntax rev^! seems like it should do the same, but it
> doesn't really do the right thing for merge commits (it gives only the
> merge itself).
.. rather than the commit and those on side branches).
>
> As a natural generalisation, we also accept rev^-n where n excludes the
> nth parent of rev,
> although this is expected to be generally less useful.
Presumptious? for a two parent merge, surely(?) rev^-2 will give you what
has been going on on the main line while the branch was being prepared...
compare A^- and A^-2.
>
> [v2: Use ^- instead of % as suggested by Junio Hamano and use some
> common helper functions for parsing.]
As others noted, stick the note below a three dash line following the sign
off (it can be part of the commit message after the sign off. It's also a
useful place for including any cc: list when using format-patch and
send-email.
>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
> Documentation/revisions.txt | 14 +++++++
> builtin/rev-parse.c | 28 ++++++++++++++
> revision.c | 91
> +++++++++++++++++++++++++++++++++++++++++++++
> revision.h | 1 +
> 4 files changed, 134 insertions(+)
>
> diff --git Documentation/revisions.txt Documentation/revisions.txt
> index 4bed5b1..6e33801 100644
> --- Documentation/revisions.txt
> +++ Documentation/revisions.txt
> @@ -281,6 +281,14 @@ is a shorthand for 'HEAD..origin' and asks "What did
> the origin do since
> I forked from them?" Note that '..' would mean 'HEAD..HEAD' which is an
> empty range that is both reachable and unreachable from HEAD.
>
> +Parent Exclusion Notation
> +~~~~~~~~~~~~~~~~~~~~~~~~~
> +The '<rev>{caret}-{<n>}', Parent Exclusion Notation::
> +Shorthand for '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
> +given. This is typically useful for merge commits where you
> +can just pass '<commit>{caret}-' to get all the commits in the branch
s/get all the/get the commit and all the/ ?
It could be misread as a way of selecting just those commits that are within
the side branch without including the given commit itself.
> +that was merged in merge commit '<commit>'.
> +
> Other <rev>{caret} Parent Shorthand Notations
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Two other shorthands exist, particularly useful for merge commits,
> @@ -316,6 +324,10 @@ Revision Range Summary
> <rev2> but exclude those that are reachable from both. When
> either <rev1> or <rev2> is omitted, it defaults to `HEAD`.
>
> +'<rev>{caret}-{<n>}', e.g. 'HEAD{caret}, HEAD{caret}-2'::
> + Equivalent to '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
> + given.
> +
> '<rev>{caret}@', e.g. 'HEAD{caret}@'::
> A suffix '{caret}' followed by an at sign is the same as listing
> all parents of '<rev>' (meaning, include anything reachable from
> @@ -339,6 +351,8 @@ spelt out:
> C I J F C
> B..C = ^B C C
> B...C = B ^F C G H D E B C
> + B^- = B^..B
> + = B ^B^1 E I J F B
> C^@ = C^1
> = F I J F
> B^@ = B^1 B^2 B^3
> diff --git builtin/rev-parse.c builtin/rev-parse.c
> index 76cf05e..ad5e6ac 100644
> --- builtin/rev-parse.c
> +++ builtin/rev-parse.c
> @@ -292,6 +292,32 @@ static int try_difference(const char *arg)
> return 0;
> }
>
> +static int try_parent_exclusion(const char *arg)
> +{
> + int ret = 0;
> + char *to_rev = NULL;
> + char *from_rev = NULL;
> + unsigned char to_sha1[20];
> + unsigned char from_sha1[20];
> +
> + if (parse_parent_exclusion(arg, &to_rev, &from_rev))
> + goto out;
> + if (get_sha1_committish(to_rev, to_sha1))
> + goto out;
> + if (get_sha1_committish(from_rev, from_sha1))
> + goto out;
> +
> + show_rev(NORMAL, to_sha1, to_rev);
> + show_rev(REVERSED, from_sha1, from_rev);
> +
> + ret = 1;
> +
> +out:
> + free(to_rev);
> + free(from_rev);
> + return ret;
> +}
> +
> static int try_parent_shorthands(const char *arg)
> {
> char *dotdot;
> @@ -839,6 +865,8 @@ int cmd_rev_parse(int argc, const char **argv, const
> char *prefix)
> /* Not a flag argument */
> if (try_difference(arg))
> continue;
> + if (try_parent_exclusion(arg))
> + continue;
> if (try_parent_shorthands(arg))
> continue;
> name = arg;
> diff --git revision.c revision.c
> index 969b3d1..0480f19 100644
> --- revision.c
> +++ revision.c
> @@ -1419,6 +1419,93 @@ static void prepare_show_merge(struct rev_info
> *revs)
> revs->limited = 1;
> }
>
> +/*
> + * If 'arg' is on the form '<rev>^-{<n>}', then return 0 and
> + * '*to_rev' and '*from_rev' will contain '<rev>' and '<rev>^<n>',
> + * respectively.
> + */
> +int parse_parent_exclusion(const char *arg, char **to_rev, char
> **from_rev)
> +{
> + char *caret;
> + unsigned int n = 1;
> +
> + /*
> + * <rev>^-{<n>} is shorthand for <rev>^<n>..<rev>, with <n> = 1 if
> + * not given. This is typically used for merge commits where you
> + * can just pass '<merge>^-' and it will show you all the commits in
> + * the branch that was merged.
> + */
> +
> + if (!(caret = strstr(arg, "^-")))
> + return 1;
> + if (caret[2]) {
> + char *end;
> + n = strtoul(&caret[2], &end, 10);
> + if (*end != '\0')
> + return 1;
> + }
> + *to_rev = xstrndup(arg, caret - arg);
> + *from_rev = xstrfmt("%s^%u", *to_rev, n);
> + return 0;
> +}
> +
> +static int handle_parent_exclusion(const char *arg, struct rev_info
> *revs, int flags)
> +{
> + int ret = 1;
> + char *to_rev = NULL;
> + char *from_rev = NULL;
> + unsigned char to_sha1[20];
> + unsigned char from_sha1[20];
> +
> + struct object *a_obj, *b_obj;
> + unsigned int flags_exclude = flags ^ (UNINTERESTING | BOTTOM);
> + unsigned int a_flags;
> +
> + /*
> + * <rev>^-{<n>} is shorthand for <rev>^<n>..<rev>, with <n> = 1 if
> + * not given. This is typically used for merge commits where you
> + * can just pass <merge>^- and it will show you all the commits in
> + * the branches that were merged.
> + */
> +
> + if (parse_parent_exclusion(arg, &to_rev, &from_rev))
> + goto out;
> +
> + if (get_sha1_committish(to_rev, to_sha1)) {
> + if (revs->ignore_missing)
> + goto out;
> + die("Unknown revision %s", to_rev);
> + }
> +
> + if (get_sha1_committish(from_rev, from_sha1)) {
> + if (revs->ignore_missing)
> + goto out;
> + die("Unknown revision %s", from_rev);
> + }
> +
> + a_obj = parse_object(from_sha1);
> + b_obj = parse_object(to_sha1);
> + if (!a_obj || !b_obj) {
> + if (revs->ignore_missing)
> + goto out;
> + die("Invalid revision range %s", arg);
> + }
> +
> + a_flags = flags_exclude;
> + a_obj->flags |= a_flags;
> + b_obj->flags |= flags;
> + add_rev_cmdline(revs, a_obj, from_rev, REV_CMD_LEFT, a_flags);
> + add_pending_object(revs, a_obj, from_rev);
> + add_rev_cmdline(revs, b_obj, to_rev, REV_CMD_RIGHT, flags);
> + add_pending_object(revs, b_obj, to_rev);
> +
> + ret = 0;
> +out:
> + free(to_rev);
> + free(from_rev);
> + return ret;
> +}
> +
> int handle_revision_arg(const char *arg_, struct rev_info *revs, int
> flags, unsigned revarg_opt)
> {
> struct object_context oc;
> @@ -1519,6 +1606,10 @@ int handle_revision_arg(const char *arg_, struct
> rev_info *revs, int flags, unsi
> }
> *dotdot = '.';
> }
> +
> + if (!handle_parent_exclusion(arg, revs, flags))
> + return 0;
> +
> dotdot = strstr(arg, "^@");
> if (dotdot && !dotdot[2]) {
> *dotdot = 0;
> diff --git revision.h revision.h
> index 9fac1a6..ca5bebc 100644
> --- revision.h
> +++ revision.h
> @@ -243,6 +243,7 @@ extern int setup_revisions(int argc, const char
> **argv, struct rev_info *revs,
> extern void parse_revision_opt(struct rev_info *revs, struct
> parse_opt_ctx_t *ctx,
> const struct option *options,
> const char * const usagestr[]);
> +extern int parse_parent_exclusion(const char *arg, char **to_rev, char
> **from_rev);
> #define REVARG_CANNOT_BE_FILENAME 01
> #define REVARG_COMMITTISH 02
> extern int handle_revision_arg(const char *arg, struct rev_info *revs,
> --
> 2.10.0.rc0.1.g07c9292
>
>
^ permalink raw reply
* Re: [PATCH 0/3] recursive support for ls-files
From: Brandon Williams @ 2016-09-25 16:32 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20160925071744.2qcz7cntc7xxpjqv@sigill.intra.peff.net>
On 09/25, Jeff King wrote:
> On Fri, Sep 23, 2016 at 05:13:31PM -0700, Brandon Williams wrote:
>
> > After looking at the feedback I rerolled a few things, in particular the
> > --submodule_prefix option that existed to give a submodule context about where
> > it had been invoked from. People didn't seem to like the idea of exposing this
> > to the users (yet anyways) so I removed it as an option and instead have it
> > being passed to a child process via an environment variable
> > GIT_INTERNAL_SUBMODULE_PREFIX. This way we don't have to support anything to
> > external users at the moment.
>
> I think we can still have it as a command-line argument and declare it
> internal. It's not like environment variables cannot also be set by our
> callers. :)
>
> I don't mind it as an environment variable, though. In some ways it
> makes things easier. I just think "internal versus external" and the
> exact implementation are orthogonal.
>
We may still want it to be an option at some point in the future. This
way we can revisit making it an option once we know more about the other
uses it could have (aside from just being for submodules as someone
suggested).
> > Also fixed a bug (and added a test) for the -z options as pointed out by Jeff
> > King.
>
> Hmm. It is broken after patch 2, and then fixed in patch 3. Usually we'd
> try not to have a broken state in the history. It's less important in
> this case, because the breakage is not a regression
> (--recurse-submodules is a new feature, so you could consider it "not
> working" until the 3rd patch). But I think it's still a good rule to
> follow, because it makes the commits easier to review, look at later,
> etc.
>
> For that matter, I do not understand why options like "-s" get enabled
> in patch 3. I do not mind them starting as disabled in patch 2, but it
> seems like "pass along some known-safe options" should be its own patch
> somewhere between patches 2 and 3.
I'll keep that in mind for future patches. I figured that since it was
fixed in the end that would be fine but if things shouldn't be broken at
any state in the patch series I'll make sure to not do that in the
future.
> There are some other options that are ignored (neither disabled nor
> passed along to children). Most of them are related to exclusions, which
> I _think_ are safe to ignore (they do not do anything interesting unless
> you specify "-o", which is explicitly disabled). I'm not sure about
> --with-tree, though (or what it would even mean in the context of
> recursing).
These other features that are disabled now could be enabled in a future
patch. You're right though I'd have to think about the --with-tree
option a bit more and what it would mean with submodules.
-Brandon
^ permalink raw reply
* Re: [PATCH v8 10/11] convert: make apply_filter() adhere to standard Git error handling
From: Jakub Narębski @ 2016-09-25 14:47 UTC (permalink / raw)
To: Lars Schneider, git
Cc: Jeff King, Junio C Hamano, Stefan Beller, Martin-Louis Bright,
Torsten Bögershausen, Ramsay Jones
In-Reply-To: <20160920190247.82189-11-larsxschneider@gmail.com>
W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> apply_filter() returns a boolean that tells the caller if it
> "did convert or did not convert". The variable `ret` was used throughout
> the function to track errors whereas `1` denoted success and `0`
> failure. This is unusual for the Git source where `0` denotes success.
>
> Rename the variable and flip its value to make the function easier
> readable for Git developers.
This also allow to use the 'err = error("<error message>");' idiom,
isn't it...
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
> convert.c | 15 ++++++---------
> 1 file changed, 6 insertions(+), 9 deletions(-)
...which allows to delete some lines of code. Very nice.
> - int ret = 1;
> + int err = 0;
> - error("read from external filter '%s' failed", cmd);
> - ret = 0;
> + err = error("read from external filter '%s' failed", cmd);
> - if (ret) {
> + if (!err) {
> - return ret;
> + return !err;
Looks good.
^ permalink raw reply
* Re: [PATCH v8 09/11] convert: modernize tests
From: Jakub Narębski @ 2016-09-25 14:43 UTC (permalink / raw)
To: Lars Schneider, git
Cc: Jeff King, Junio C Hamano, Stefan Beller, Martin-Louis Bright,
Torsten Bögershausen, Ramsay Jones
In-Reply-To: <20160920190247.82189-10-larsxschneider@gmail.com>
W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> Use `test_config` to set the config, check that files are empty with
> `test_must_be_empty`, compare files with `test_cmp`, and remove spaces
> after ">" and "<".
That's good.
>
> Please note that the "rot13" filter configured in "setup" keeps using
> `git config` instead of `test_config` because subsequent tests might
> depend on it.
This is good information to have for doing review (which could include
"post-mortem" review during bisect, so it should be in commit message
proper).
>
> Reviewed-by: Stefan Beller <sbeller@google.com>
I have not reviewed this patch in detail, but it looks good.
A bit of nitpicking below.
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
> t/t0021-conversion.sh | 58 +++++++++++++++++++++++++--------------------------
> 1 file changed, 29 insertions(+), 29 deletions(-)
>
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index e799e59..dc50938 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -38,8 +38,8 @@ script='s/^\$Id: \([0-9a-f]*\) \$/\1/p'
>
> test_expect_success check '
This patch is "while at it" already for this patch series, done
I guess for new tests to both use modern style, and be consistent
with the rest of test...
...that said, if you could modernize _naming_ of tests. The t0021
test is quite inconsistent, and uses:
* standard short names, like 'setup', without quotes (once),
which is I think all right
* cryptic short names, like 'check', without quotes (once)
* snake_case name, like 'expanded_in_repo', without quotes (once)
> test_expect_success "filter: clean empty file" '
> test_expect_success "filter: smudge empty file" '
* double quoted names (twice, see above)
* proper modern names, with single quotes (the rest),
which is as almost all the rest should be using
Best,
--
Jakub Narębski
^ permalink raw reply
* git-pack-objects(1): --honor-pack-keep
From: Anatoly Borodin @ 2016-09-25 14:42 UTC (permalink / raw)
To: git
Hi All,
the option `--honor-pack-keep` is not mentioned in the `SYNOPSIS` of
`git-pack-objects(1)` (`Documentation/git-pack-objects.txt`), only below
in the `OPTIONS`.
The same story with `--include-tag`, `--window-memory`,
`--max-pack-size`, `--no-reuse-object`, `--compression`, `--thin`,
`--threads`, and `--index-version`.
Is it a mistake, or different options should be treated differently,
like, often used vs. seldom used? But as far as I can see,
`--honor-pack-keep` is used pretty often by `git gc`.
PS The `--non-empty` block is indented with whitespaces, should it be
corrected?
--
Mit freundlichen Grüßen,
Anatoly Borodin
^ permalink raw reply
* Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0
From: Gustavo Grieco @ 2016-09-25 14:12 UTC (permalink / raw)
To: git
In-Reply-To: <1399913289.8224468.1474810664933.JavaMail.zimbra@imag.fr>
Hi,
We found a stack read out-of-bounds parsing object files using git 2.10.0. It was tested on ArchLinux x86_64. To reproduce, first recompile git with ASAN support and then execute:
$ git init ; mkdir -p .git/objects/b2 ; printf 'x' > .git/objects/b2/93584ddd61af21260be75ee9f73e9d53f08cd0
Finally you can trigger the bug using several commands from git (other commands that parses all objects will work too), for instance:
$ git fsck
The ASAN report is here:
==2763==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffe16e4a690 at pc 0x0000006fe5dc bp 0x7ffe16e4a530 sp 0x7ffe16e4a520
READ of size 1 at 0x7ffe16e4a690 thread T0
#0 0x6fe5db in parse_sha1_header_extended /home/g/Work/Code/git-2.10.0/sha1_file.c:1684
#1 0x702cd4 in sha1_loose_object_info /home/g/Work/Code/git-2.10.0/sha1_file.c:2660
#2 0x70332c in sha1_object_info_extended /home/g/Work/Code/git-2.10.0/sha1_file.c:2696
#3 0x7038e0 in sha1_object_info /home/g/Work/Code/git-2.10.0/sha1_file.c:2745
#4 0x648498 in parse_object /home/g/Work/Code/git-2.10.0/object.c:260
#5 0x48d46d in fsck_sha1 builtin/fsck.c:367
#6 0x48da47 in fsck_loose builtin/fsck.c:493
#7 0x707514 in for_each_file_in_obj_subdir /home/g/Work/Code/git-2.10.0/sha1_file.c:3477
#8 0x70775b in for_each_loose_file_in_objdir_buf /home/g/Work/Code/git-2.10.0/sha1_file.c:3512
#9 0x707885 in for_each_loose_file_in_objdir /home/g/Work/Code/git-2.10.0/sha1_file.c:3532
#10 0x48dc1d in fsck_object_dir builtin/fsck.c:521
#11 0x48e2e6 in cmd_fsck builtin/fsck.c:644
#12 0x407a8f in run_builtin /home/g/Work/Code/git-2.10.0/git.c:352
#13 0x407e35 in handle_builtin /home/g/Work/Code/git-2.10.0/git.c:539
#14 0x408175 in run_argv /home/g/Work/Code/git-2.10.0/git.c:593
#15 0x408458 in cmd_main /home/g/Work/Code/git-2.10.0/git.c:665
#16 0x53fc70 in main /home/g/Work/Code/git-2.10.0/common-main.c:40
#17 0x7f0f46d43290 in __libc_start_main (/usr/lib/libc.so.6+0x20290)
#18 0x405209 in _start (/home/g/Work/Code/git-2.10.0/git+0x405209)
Address 0x7ffe16e4a690 is located in stack of thread T0 at offset 192 in frame
#0 0x702834 in sha1_loose_object_info /home/g/Work/Code/git-2.10.0/sha1_file.c:2614
This frame has 5 object(s):
[32, 40) 'mapsize'
[96, 120) 'hdrbuf'
[160, 192) 'hdr' <== Memory access at offset 192 overflows this variable
[224, 368) 'st'
[416, 576) 'stream'
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
(longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow /home/g/Work/Code/git-2.10.0/sha1_file.c:1684 in parse_sha1_header_extended
Shadow bytes around the buggy address:
0x100042dc1480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x100042dc1490: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x100042dc14a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x100042dc14b0: 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 f4
0x100042dc14c0: f4 f4 f2 f2 f2 f2 00 00 00 f4 f2 f2 f2 f2 00 00
=>0x100042dc14d0: 00 00[f2]f2 f2 f2 00 00 00 00 00 00 00 00 00 00
0x100042dc14e0: 00 00 00 00 00 00 00 00 f4 f4 f2 f2 f2 f2 00 00
0x100042dc14f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x100042dc1500: 00 00 f3 f3 f3 f3 00 00 00 00 00 00 00 00 00 00
0x100042dc1510: 00 00 00 00 f1 f1 f1 f1 04 f4 f4 f4 f2 f2 f2 f2
0x100042dc1520: 00 00 00 00 00 f4 f4 f4 f3 f3 f3 f3 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Heap right redzone: fb
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack partial redzone: f4
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
Regards,
Gustavo.
^ permalink raw reply
* Re: [RFC PATCH v2] revision: new rev^-n shorthand for rev^n..rev
From: Jakub Narębski @ 2016-09-25 14:19 UTC (permalink / raw)
To: Vegard Nossum, git
Cc: Junio C Hamano, Santi Béjar, Kevin Bracey, Philip Oakley
In-Reply-To: <20160925085511.12515-1-vegard.nossum@oracle.com>
W dniu 25.09.2016 o 10:55, Vegard Nossum pisze:
> I use rev^..rev daily, and I'm surely not the only one. To save typing
> (or copy-pasting, if the rev is long -- like a full SHA-1 or branch name)
> we can make rev^- a shorthand for that.
>
> The existing syntax rev^! seems like it should do the same, but it
> doesn't really do the right thing for merge commits (it gives only the
> merge itself).
>
> As a natural generalisation, we also accept rev^-n where n excludes the
> nth parent of rev, although this is expected to be generally less useful.
>
> [v2: Use ^- instead of % as suggested by Junio Hamano and use some
> common helper functions for parsing.]
Minor sidenote: the above should go after the "---" line, as it should
be not included in the final commit message.
>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
> Documentation/revisions.txt | 14 +++++++
> builtin/rev-parse.c | 28 ++++++++++++++
> revision.c | 91 +++++++++++++++++++++++++++++++++++++++++++++
> revision.h | 1 +
> 4 files changed, 134 insertions(+)
^ permalink raw reply
* Re: Journal of Failed Git Experiments, Volume 1
From: Johannes Schindelin @ 2016-09-25 14:17 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20160914235547.h3n2otje2hec6u7k@sigill.intra.peff.net>
Hi Peff,
On Wed, 14 Sep 2016, Jeff King wrote:
> I try a lot of different experiments with git performance, some of them
> more hare-brained than others. The ones that succeed end up as real
> patches. But I hate for the ones that fail to die a quiet death. Then
> nobody learns what _doesn't_ work, and nobody has the opportunity to
> point out the spot where I made a stupid mistake that invalidates the
> whole result.
To show those experiments, with analysis, is a really good idea.
I found the zstd experiment in particular very educating, as I wondered
about the same: could we maybe use it to accelerate Git operations? Now I
know.
Ciao,
Dscho
^ permalink raw reply
* Re: [RFC PATCH v2] revision: new rev^-n shorthand for rev^n..rev
From: Ramsay Jones @ 2016-09-25 14:07 UTC (permalink / raw)
To: Vegard Nossum, git
Cc: Junio C Hamano, Santi Béjar, Kevin Bracey, Philip Oakley
In-Reply-To: <20160925085511.12515-1-vegard.nossum@oracle.com>
On 25/09/16 09:55, Vegard Nossum wrote:
> I use rev^..rev daily, and I'm surely not the only one. To save typing
> (or copy-pasting, if the rev is long -- like a full SHA-1 or branch name)
> we can make rev^- a shorthand for that.
>
> The existing syntax rev^! seems like it should do the same, but it
> doesn't really do the right thing for merge commits (it gives only the
> merge itself).
>
> As a natural generalisation, we also accept rev^-n where n excludes the
> nth parent of rev, although this is expected to be generally less useful.
>
> [v2: Use ^- instead of % as suggested by Junio Hamano and use some
> common helper functions for parsing.]
I would place this v2 commentary below the '---' marker (so that it
won't appear in the commit message) ...
>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
... here.
> Documentation/revisions.txt | 14 +++++++
> builtin/rev-parse.c | 28 ++++++++++++++
> revision.c | 91 +++++++++++++++++++++++++++++++++++++++++++++
> revision.h | 1 +
> 4 files changed, 134 insertions(+)
>
> diff --git Documentation/revisions.txt Documentation/revisions.txt
> index 4bed5b1..6e33801 100644
> --- Documentation/revisions.txt
> +++ Documentation/revisions.txt
> @@ -281,6 +281,14 @@ is a shorthand for 'HEAD..origin' and asks "What did the origin do since
> I forked from them?" Note that '..' would mean 'HEAD..HEAD' which is an
> empty range that is both reachable and unreachable from HEAD.
>
> +Parent Exclusion Notation
> +~~~~~~~~~~~~~~~~~~~~~~~~~
> +The '<rev>{caret}-{<n>}', Parent Exclusion Notation::
> +Shorthand for '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
> +given. This is typically useful for merge commits where you
> +can just pass '<commit>{caret}-' to get all the commits in the branch
> +that was merged in merge commit '<commit>'.
> +
> Other <rev>{caret} Parent Shorthand Notations
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Two other shorthands exist, particularly useful for merge commits,
> @@ -316,6 +324,10 @@ Revision Range Summary
> <rev2> but exclude those that are reachable from both. When
> either <rev1> or <rev2> is omitted, it defaults to `HEAD`.
>
> +'<rev>{caret}-{<n>}', e.g. 'HEAD{caret}, HEAD{caret}-2'::
missing '-' ------------------------------^
> + Equivalent to '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
> + given.
> +
> '<rev>{caret}@', e.g. 'HEAD{caret}@'::
> A suffix '{caret}' followed by an at sign is the same as listing
> all parents of '<rev>' (meaning, include anything reachable from
> @@ -339,6 +351,8 @@ spelt out:
> C I J F C
> B..C = ^B C C
> B...C = B ^F C G H D E B C
> + B^- = B^..B
> + = B ^B^1 E I J F B
> C^@ = C^1
> = F I J F
> B^@ = B^1 B^2 B^3
> diff --git builtin/rev-parse.c builtin/rev-parse.c
> index 76cf05e..ad5e6ac 100644
> --- builtin/rev-parse.c
> +++ builtin/rev-parse.c
> @@ -292,6 +292,32 @@ static int try_difference(const char *arg)
> return 0;
> }
>
> +static int try_parent_exclusion(const char *arg)
> +{
> + int ret = 0;
> + char *to_rev = NULL;
> + char *from_rev = NULL;
> + unsigned char to_sha1[20];
> + unsigned char from_sha1[20];
As Matthieu already mentioned, maybe use 'struct object_id' here.
> +
> + if (parse_parent_exclusion(arg, &to_rev, &from_rev))
> + goto out;
> + if (get_sha1_committish(to_rev, to_sha1))
... then 'to_sha1.hash' here, etc ...
ATB,
Ramsay Jones
^ 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