Re: [PATCH v3] reflog-walk: don't segfault on non-commit sha1's in the reflog
On wo, 2015-12-30 at 16:02 -0800, Junio C Hamano wrote:
> Dennis Kaarsemaker writes:
>
> > diff --git a/reflog-walk.c b/reflog-walk.c
> > index 85b8a54..0ebd1da 100644
> > --- a/reflog-walk.c
> > +++ b/reflog-walk.c
> > @@ -221,6 +221,7 @@ void fake_reflog_parent(struct reflog_walk_info
> > *info, struct commit *commit)
> > struct commit_info *commit_info =
> > get_commit_info(commit, &info->reflogs, 0);
> > struct commit_reflog *commit_reflog;
> > + struct object *logobj;
>
> This thing is not initialized...
>
> > struct reflog_info *reflog;
> >
> > info->last_commit_reflog = NULL;
> > @@ -232,15 +233,20 @@ void fake_reflog_parent(struct
> > reflog_walk_info *info, struct commit *commit)
> > commit->parents = NULL;
> > return;
> > }
> > -
> > - reflog = &commit_reflog->reflogs->items[commit_reflog
> > ->recno];
> > info->last_commit_reflog = commit_reflog;
> > - commit_reflog->recno--;
> > - commit_info->commit = (struct commit *)parse_object(reflog
> > ->osha1);
> > - if (!commit_info->commit) {
> > +
> > + do {
> > + reflog = &commit_reflog->reflogs
> > ->items[commit_reflog->recno];
> > + commit_reflog->recno--;
> > + logobj = parse_object(reflog->osha1);
> > + } while (commit_reflog->recno && (logobj && logobj->type
> > != OBJ_COMMIT));
>
> But this loop runs at least once, so logobj will always have some
> sane value when the loop exits.
>
> > + if (!logobj || logobj->type != OBJ_COMMIT) {
>
> And the only case where this should trigger is when we ran out of
> recno. Am I reading the updated code correctly?
Yes, your description matches what I tried to implement.
> With the updated code, the number of times we return from this
> function is different from the number initially set to recno. I had
> to wonder if the caller cares and misbehaves, but the caller does
> not know how long the reflog is before starting to call
> get_revision() in a loop anyway, so it already has to deal with a
> case where it did .recno=20 and get_revision() did not return that
> many times. So this probably is safe.
That corresponds to what I see when experimenting with different
reflogs.
> > +test_expect_success 'reflog containing non-commit sha1s displays
> > properly' '
>
> In general, "properly" is a poor word to use in test description (or
> a commit log message or a bug report, for that matter), as the whole
> point of a test is to precisely define what is "proper".
>
> And the code change declares that a proper thing to do is to omit
> non-commit entries without segfaulting, so something like
>
> s/displays properly/omits them/
>
> perhaps?
I did find the test title a bit iffy but couldn't really figure out
why. What you're saying makes a lot of sense, will fix.
--
Dennis Kaarsemaker
www.kaarsemaker.net
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC v2 0/2] add regex match flags to git describe
On 12/31/2015 01:23 AM, Junio C Hamano wrote: Mostyn Bramley-Moore writes: OK, brainstorming a bit, how about either of these: 1) --match-pattern-type= It's a bit lengthy (maybe --match-type would be sufficient), but I like that the value names are shared with git grep etc option names. And it seems future-proof- if we ever need to support different pattern types for other arguments, a --foo-pattern-type flag could be added and make obvious sense. Swapping the option key and value may not be a bad idea, but one problem that the above does not solve, which I outlined in the message you are responding to, is that "match-pattern-type" does not give any hint that this is about affecting the match that is done to "refs", e.g. you cannot tell in $ git mgrep --match-pattern-type=perl-regexp -e foo --refs 'release_*' if the perl-regexp is to be used for matching branch names or for matching the strings the command looks for in the trees of the matching branches. There is a hint: --foo-pattern-type applies only to --foo. In your mgrep example --match-pattern-type would apply to --match patterns only, and if we choose to implement it then --refs-pattern-type would apply to --refs patterns only. eg: $ git mgrep --match '^foo' --match-pattern-type=perl-regexp --refs 'release_*' --refs-pattern-type=glob Magic pattern annotation like we do for pathspecs Duy raised may not be a bad idea, either, and would probably be easier to teach people. Just like in Perl "(?i)$any_pattern" is a way to introduce the case insensitive match with $any_pattern, we may be able to pick an extensible magic syntax and decorate the pattern you would specify for matching refnames to tell Git what kind of pattern it is, e.g. $ git mgrep -P -e foo --refs '/?glob/release_*' I am not suggesting that we must use /?/ prefix as the "extensible magic syntax" here--I am just illustrating what I mean by "extensible magic syntax". I hadn't seen the pathspec magic patterns before- interesting. We could possibly share syntax with pathspecs, ie :(?pattern-options...)pattern Would this be confusing for commands that already have --perl-regexp etc? What should happen if you specify both --perl-regexp and and a different type of pattern like :(glob)foo (error out, I suppose)? -Mostyn. -- Mostyn Bramley-Moore TV and Connected Devices Opera Software ASA most...@opera.com -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ./t3310-notes-merge-manual-resolve.sh broken on pu under Mac OS ?
On 31/12/15 06:08, Eric Sunshine wrote:
> On Wed, Dec 30, 2015 at 8:20 AM, Torsten Bögershausen wrote:
>> I got 2 failures on pu under Mac OS, (Linux is OK)
>> I did some very basic debugging, it seems as if grep doesn't find
>> a needed string.
>> Does anybody have an idea here ?
>
> I'm unable to reproduce these failures on Mac.
>
This test failed during a short window (due to commit 2bd811ec) and
has already been fixed in commit 3a74ea38 ("notes: allow merging
from arbitrary references", 29-12-2015).
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 09/10] config: add core.untrackedCache
On Thu, Dec 31, 2015 at 12:23 AM, Junio C Hamano wrote: > Christian Couder writes: > >> On Tue, Dec 29, 2015 at 11:35 PM, Junio C Hamano wrote: >> ... >>> While the above is not wrong per-se, from the point of those who >>> looked for these options (that is, those who wanted to do a one-shot >>> enabling or disabling of the feature, perhaps to try it out to see >>> how well it helps on their system), I think the explanation of the >>> interaction between the option and the config is backwards. For >>> their purpose, setting it to `true` or `false` will be hinderance, >>> because the options are made no-op by such a setting. IOW, the >>> advertisement "once you decided that you want to use the feature, >>> configuration is a better place to set it" does not belong here. >>> >>> If I were writing this documentation, I'd probably rephrase the >>> second paragraph like so: >>> >>> These options take effect only when the >>> `core.untrackedCache` configuration variable (see >>> linkgit:git-config[1]) is set to `keep` (or it is left >>> unset). When the configuration variable is set to `true`, >>> the untracked cache feature is always enabled (and when it >>> is set to `false`, it is always disabled), making these >>> options no-op. >>> >>> perhaps. >> >> Yeah, but "no-op" is not technically true, as if you just set the >> config variable to true for example and then use "--untracked-cache" >> then the cache will be added immediately. > > The "update-index --[no-]untracked-cache" command is made to no > longer follow the user's immediate desire (i.e. enable or disable) > expressed by the invocation of the command. That is what I meant by > 'no-op'. > >> ... for example "git update-index --untracked-cache" will die() if >> the config variable is set to false. > > As I think it is a BUG to make these die(), it is an irrelevant > objection ;-). Ok I think I will just use what you suggest except the "no-op" part: These options take effect only when the `core.untrackedCache` configuration variable (see linkgit:git-config[1]) is set to `keep` (or it is left unset). When the configuration variable is set to `true`, the untracked cache feature is always enabled (and when it is set to `false`, it is always disabled). >>> I somehow thought, per the previous discussion with Duy, that the >>> check and addition of an empty one (if missing in the index and >>> configuration is set to `true`) or removal of an existing one (if >>> configuration is set to `false`) were to be done when the index is >>> read by read_index_from(), though. If you have to say "After >>> setting the configuration, you must run this command", that does not >>> sound like a huge improvement over "If you want to enable this >>> feature, you must run this command". >> >> The untracked-cache feature, as I tried to explain in an email in the >> previous discussion, has an effect only on git status when it has to >> show untracked files. Otherwise the untracked-cache is simply not >> used. It might be a goal to use it more often, but it's not my patch >> series' goal. > > In the future we may extend the system to allow "git add somedir/" > to take advantage of the untracked cache (i.e. we may already know > what untracked paths there are without letting readdir(3) to scan it > in somedir/), for example. Is it reasonable to force users to say > "git status", in such a future? If a new feature takes advantage of the untracked cache, it is reasonable that this feature enables or disables it if should be enabled or disabled. Maybe we can just say in the doc something like: When the `core.untrackedCache` configuration variable is changed, the untracked cache is added to or removed from the index the next time a command that can use the untracked cache is run; while when `--[no-|force-]untracked-cache` are used, the untracked cache is immediately added to or removed from the index. Currently only "git status" can use the untracked cache. > And more importantly, when that > future comes, is it reasonable to force users to re-learn Git to do > something else to do things differently at that point? I don't think people will have to relearn anything or do things differently especially if things are explained like above. If they want to use the untracked cache by setting a config option, and then want to enable it right away, they can still use git status for that, so things will continue to work. What scenario do you have in mind where people would have to do things differently? > Itt sounds like somewhat a short-sighted mindset to design the > system, and I was hoping that by now you would have become better > than that. > > The real question is what are the problems in implementing this in > the way Duy suggested in the previous discussion. The answer may > fall into somewhere between "that approach does not work in such and > su
A failed attempt to integrate diff-highlight to the core
This is mostly for the record. The patches are not as important as the result in this mail. But if you want to try out, you can. If you don't know, the script diff-highlight in contrib can highlight changes between two nearly identical lines, pointing out what characters are different. It can improve readability a lot in certain cases (e.g. when you wrap _() around strings). I approached it differently. I took --color-words and reformatted its output in unified diff format. So you'll see +/- lines like a normal diff, but deleted and added words are also highlighted. The idea seems to fit well into diff-words machinery. But the output does not, especially in cases where there are many deleted/added words in a chunk. It could look.. patchy, like somebody throwing colors on a wall (especially when you do --highlight-words=.). You can try out and see. git-log works so you can see lots of diff. I don't know, maybe the idea can be improved to have something closer to diff-highlight. I suppose we can select similar lines and diff them, instead of diff the entire chunk in one go. I might revisit it at some point in future, but for now, it's a failure. Nguyễn Thái Ngọc Duy (7): diff.c: keep all word diff structs together diff.c: refactor diff_words_append() diff --color-words: another special diff case diff.c: refactor fn_out_diff_words_write_helper() diff: unified diff with colored words, step 1, unified diff only diff.c: add new arguments to emit_line_0() diff --highlight-words: actually highlight words diff.c | 400 ++--- diff.h | 3 +- 2 files changed, 363 insertions(+), 40 deletions(-) -- 2.3.0.rc1.137.g477eb31 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/7] diff: unified diff with colored words, step 1, unified diff only
The goal is to produce a unified diff, but with changed words colored
differently. A new diff-words mode is added that can keep track of both
lines and words of each chunk. The marks then are post processed and
each line is output in unified format. The actual word coloring comes in
the next patch.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
diff.c | 256 -
diff.h | 3 +-
2 files changed, 255 insertions(+), 4 deletions(-)
diff --git a/diff.c b/diff.c
index 8a9e42f..3b7317e 100644
--- a/diff.c
+++ b/diff.c
@@ -440,6 +440,23 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t
*mf2,
ecbdata->blank_at_eof_in_postimage = (at - l2) + 1;
}
+#define TAG_BEGIN_WORD(tag) \
+ ((tag) == TAG_BEGIN_OLD_WORD || \
+(tag) == TAG_BEGIN_NEW_WORD)
+
+enum pointer_tag { /* order is important because it's used in sorting */
+ TAG_END_WORD,
+ TAG_END_LINE,
+ TAG_BEGIN_LINE,
+ TAG_BEGIN_OLD_WORD,
+ TAG_BEGIN_NEW_WORD
+};
+
+struct tagged_pointer {
+ const char *str;
+ enum pointer_tag tag;
+};
+
static void emit_line_0(struct diff_options *o, const char *set, const char
*reset,
int first, const char *line, int len)
{
@@ -757,6 +774,10 @@ struct diff_words_buffer {
const char *begin, *end;
} *orig;
int orig_nr, orig_alloc;
+ unsigned long *line;
+ int line_nr, line_alloc;
+ struct tagged_pointer *mark;
+ int mark_nr, mark_alloc;
};
struct diff_words_style_elem {
@@ -772,6 +793,8 @@ struct diff_words_style {
const char *newline;
};
+static struct diff_words_style diff_words_unified_style;
+
static struct diff_words_style diff_words_styles[] = {
{ DIFF_WORDS_PORCELAIN, {"+", "\n"}, {"-", "\n"}, {" ", "\n"}, "~\n" },
{ DIFF_WORDS_PLAIN, {"{+", "+}"}, {"[-", "-]"}, {"", ""}, "\n" },
@@ -803,6 +826,13 @@ static void diff_words_append(struct diff_words_data
*diff_words,
line++;
len--;
memcpy(buffer->text.ptr + buffer->text.size, line, len);
+ if (diff_words->type == DIFF_WORDS_UNIFIED) {
+ unsigned long *l;
+ ALLOC_GROW(buffer->line, (buffer->line_nr + 1) * 2,
buffer->line_alloc);
+ l = buffer->line + (buffer->line_nr++) * 2;
+ l[0] = buffer->text.size;
+ l[1] = l[0] + len;
+ }
buffer->text.size += len;
buffer->text.ptr[buffer->text.size] = '\0';
}
@@ -816,6 +846,40 @@ static int fn_out_diff_words_write_helper(struct
diff_words_data *dw,
FILE *fp = dw->opt->file;
int print = 0;
+ if (dw->type == DIFF_WORDS_UNIFIED) {
+ struct diff_words_style *st = &diff_words_unified_style;
+ struct diff_words_buffer *b;
+ enum pointer_tag tag;
+ struct tagged_pointer *tp;
+
+ if (st_el == &st->ctx)
+ return 0;
+ else if (st_el == &st->old)
+ tag = TAG_BEGIN_OLD_WORD;
+ else if (st_el == &st->new)
+ tag = TAG_BEGIN_NEW_WORD;
+ else
+ return -1;
+
+ if (buf >= dw->minus.text.ptr &&
+ buf < dw->minus.text.ptr + dw->minus.text.size)
+ b = &dw->minus;
+ else if (buf >= dw->plus.text.ptr &&
+buf < dw->plus.text.ptr + dw->plus.text.size)
+ b = &dw->plus;
+ else
+ return -1;
+
+ ALLOC_GROW(b->mark, b->mark_nr + 2, b->mark_alloc);
+ tp = b->mark + b->mark_nr;
+ tp[0].str = buf;
+ tp[0].tag = tag;
+ tp[1].str = buf + count;
+ tp[1].tag = TAG_END_WORD;
+ b->mark_nr += 2;
+ return 0;
+ }
+
while (count) {
char *p = memchr(buf, '\n', count);
if (print)
@@ -1014,6 +1078,23 @@ static void diff_words_fill(struct diff_words_buffer
*buffer, mmfile_t *out,
}
}
+static void diff_words_add_line(struct diff_words_buffer *buffer)
+{
+ int i;
+
+ ALLOC_GROW(buffer->mark, buffer->line_nr * 2, buffer->mark_alloc);
+ for (i = 0; i < buffer->line_nr; i++) {
+ struct tagged_pointer *tp = buffer->mark + buffer->mark_nr;
+ tp->str = buffer->text.ptr + buffer->line[i * 2];
+ tp->tag = TAG_BEGIN_LINE;
+ tp++;
+ tp->str = buffer->text.ptr + buffer->line[i * 2 + 1];
+ tp->tag = TAG_END_LINE;
+ buffer->mark_nr += 2;
+ }
+ buffer->line_nr = 0;
+}
+
/* this executes the word diff on the accumulated buffers */
static void diff_words_show(struct diff_words_data *diff_words)
{
@@ -1025,6 +1106,16 @@ static void diff_words_show(struct diff_words_data
*diff
[PATCH 1/7] diff.c: keep all word diff structs together
Signed-off-by: Nguyễn Thái Ngọc Duy
---
diff.c | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/diff.c b/diff.c
index 80eb0c2..dfbed41 100644
--- a/diff.c
+++ b/diff.c
@@ -759,17 +759,6 @@ struct diff_words_buffer {
int orig_nr, orig_alloc;
};
-static void diff_words_append(char *line, unsigned long len,
- struct diff_words_buffer *buffer)
-{
- ALLOC_GROW(buffer->text.ptr, buffer->text.size + len, buffer->alloc);
- line++;
- len--;
- memcpy(buffer->text.ptr + buffer->text.size, line, len);
- buffer->text.size += len;
- buffer->text.ptr[buffer->text.size] = '\0';
-}
-
struct diff_words_style_elem {
const char *prefix;
const char *suffix;
@@ -799,6 +788,17 @@ struct diff_words_data {
struct diff_words_style *style;
};
+static void diff_words_append(char *line, unsigned long len,
+ struct diff_words_buffer *buffer)
+{
+ ALLOC_GROW(buffer->text.ptr, buffer->text.size + len, buffer->alloc);
+ line++;
+ len--;
+ memcpy(buffer->text.ptr + buffer->text.size, line, len);
+ buffer->text.size += len;
+ buffer->text.ptr[buffer->text.size] = '\0';
+}
+
static int fn_out_diff_words_write_helper(FILE *fp,
struct diff_words_style_elem *st_el,
const char *newline,
--
2.3.0.rc1.137.g477eb31
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/7] diff.c: refactor diff_words_append()
Signed-off-by: Nguyễn Thái Ngọc Duy
---
diff.c | 21 -
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/diff.c b/diff.c
index dfbed41..8af1df1 100644
--- a/diff.c
+++ b/diff.c
@@ -788,9 +788,17 @@ struct diff_words_data {
struct diff_words_style *style;
};
-static void diff_words_append(char *line, unsigned long len,
- struct diff_words_buffer *buffer)
+static void diff_words_append(struct diff_words_data *diff_words,
+ char *line, unsigned long len)
{
+ struct diff_words_buffer *buffer;
+
+ if (line[0] == '-')
+ buffer = &diff_words->minus;
+ else {
+ assert(line[0] == '+');
+ buffer = &diff_words->plus;
+ }
ALLOC_GROW(buffer->text.ptr, buffer->text.size + len, buffer->alloc);
line++;
len--;
@@ -1252,13 +1260,8 @@ static void fn_out_consume(void *priv, char *line,
unsigned long len)
}
if (ecbdata->diff_words) {
- if (line[0] == '-') {
- diff_words_append(line, len,
- &ecbdata->diff_words->minus);
- return;
- } else if (line[0] == '+') {
- diff_words_append(line, len,
- &ecbdata->diff_words->plus);
+ if (line[0] == '-' || line[0] == '+') {
+ diff_words_append(ecbdata->diff_words, line, len);
return;
} else if (starts_with(line, "\\ ")) {
/*
--
2.3.0.rc1.137.g477eb31
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/7] diff --color-words: another special diff case
Signed-off-by: Nguyễn Thái Ngọc Duy
---
diff.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/diff.c b/diff.c
index 8af1df1..1354368 100644
--- a/diff.c
+++ b/diff.c
@@ -1037,6 +1037,16 @@ static void diff_words_show(struct diff_words_data
*diff_words)
diff_words->minus.text.size = 0;
return;
}
+ /* special case: only addition */
+ if (!diff_words->minus.text.size) {
+ fputs(line_prefix, diff_words->opt->file);
+ fn_out_diff_words_write_helper(diff_words->opt->file,
+ &style->new, style->newline,
+ diff_words->plus.text.size,
+ diff_words->plus.text.ptr, line_prefix);
+ diff_words->plus.text.size = 0;
+ return;
+ }
diff_words->current_plus = diff_words->plus.text.ptr;
diff_words->last_minus = 0;
--
2.3.0.rc1.137.g477eb31
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 7/7] diff --highlight-words: actually highlight words
Signed-off-by: Nguyễn Thái Ngọc Duy
---
diff.c | 53 ++---
1 file changed, 50 insertions(+), 3 deletions(-)
diff --git a/diff.c b/diff.c
index 47d22e3..68a847d 100644
--- a/diff.c
+++ b/diff.c
@@ -457,6 +457,50 @@ struct tagged_pointer {
enum pointer_tag tag;
};
+static void emit_tagged_line(struct diff_options *o, const char *set,
+const char *line, int len,
+struct tagged_pointer *begin,
+struct tagged_pointer *end)
+{
+ struct tagged_pointer *current;
+ FILE *file = o->file;
+ const char *last_color = set;
+
+ for (current = begin; current < end; current++) {
+ struct tagged_pointer *next = current + 1;
+ const char *start = current->str;
+ const char *end = next->str;
+ const char *color = NULL;
+
+ switch (current->tag) {
+ case TAG_END_WORD:
+ color = set;
+ break;
+
+ case TAG_BEGIN_OLD_WORD:
+ color = GIT_COLOR_BG_RED GIT_COLOR_YELLOW;
+ break;
+
+ case TAG_BEGIN_NEW_WORD:
+ color = GIT_COLOR_BG_GREEN GIT_COLOR_BLUE;
+ break;
+
+ default:
+ break;
+ }
+
+ if (color && color != last_color) {
+ if (color == set)
+ fputs(GIT_COLOR_RESET, file);
+ fputs(color, file);
+ last_color = color;
+ }
+ while (end > start && end[-1] == '\n')
+ end--;
+ fwrite(start, end - start, 1, file);
+ }
+}
+
static void emit_line_0(struct diff_options *o, const char *set, const char
*reset,
int first, const char *line, int len,
struct tagged_pointer *begin,
@@ -487,7 +531,10 @@ static void emit_line_0(struct diff_options *o, const char
*set, const char *res
fputs(set, file);
if (!nofirst)
fputc(first, file);
- fwrite(line, len, 1, file);
+ if (begin)
+ emit_tagged_line(o, set, line, len, begin, end);
+ else
+ fwrite(line, len, 1, file);
fputs(reset, file);
}
if (has_trailing_carriage_return)
@@ -531,7 +578,7 @@ static void emit_line_checked(const char *reset,
ws = NULL;
}
- if (!ws)
+ if (!ws || begin)
emit_line_0(ecbdata->opt, set, reset, sign,
line, len, begin, end);
else if (sign == '+' && new_blank_line_at_eof(ecbdata, line, len))
@@ -1312,7 +1359,7 @@ static void diff_words_flush_unified(struct emit_callback
*ecb,
emit_line_checked(reset, ecb, begin_line->str,
end_line->str - begin_line->str,
color, ws_error_highlight, sign,
- NULL, NULL);
+ begin_line, end_line);
}
b->mark_nr = 0;
}
--
2.3.0.rc1.137.g477eb31
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/7] diff.c: refactor fn_out_diff_words_write_helper()
Signed-off-by: Nguyễn Thái Ngọc Duy
---
diff.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/diff.c b/diff.c
index 1354368..8a9e42f 100644
--- a/diff.c
+++ b/diff.c
@@ -807,12 +807,13 @@ static void diff_words_append(struct diff_words_data
*diff_words,
buffer->text.ptr[buffer->text.size] = '\0';
}
-static int fn_out_diff_words_write_helper(FILE *fp,
+static int fn_out_diff_words_write_helper(struct diff_words_data *dw,
struct diff_words_style_elem *st_el,
const char *newline,
size_t count, const char *buf,
const char *line_prefix)
{
+ FILE *fp = dw->opt->file;
int print = 0;
while (count) {
@@ -919,7 +920,7 @@ static void fn_out_diff_words_aux(void *priv, char *line,
unsigned long len)
fputs(line_prefix, diff_words->opt->file);
}
if (diff_words->current_plus != plus_begin) {
- fn_out_diff_words_write_helper(diff_words->opt->file,
+ fn_out_diff_words_write_helper(diff_words,
&style->ctx, style->newline,
plus_begin - diff_words->current_plus,
diff_words->current_plus, line_prefix);
@@ -927,13 +928,13 @@ static void fn_out_diff_words_aux(void *priv, char *line,
unsigned long len)
fputs(line_prefix, diff_words->opt->file);
}
if (minus_begin != minus_end) {
- fn_out_diff_words_write_helper(diff_words->opt->file,
+ fn_out_diff_words_write_helper(diff_words,
&style->old, style->newline,
minus_end - minus_begin, minus_begin,
line_prefix);
}
if (plus_begin != plus_end) {
- fn_out_diff_words_write_helper(diff_words->opt->file,
+ fn_out_diff_words_write_helper(diff_words,
&style->new, style->newline,
plus_end - plus_begin, plus_begin,
line_prefix);
@@ -1030,7 +1031,7 @@ static void diff_words_show(struct diff_words_data
*diff_words)
/* special case: only removal */
if (!diff_words->plus.text.size) {
fputs(line_prefix, diff_words->opt->file);
- fn_out_diff_words_write_helper(diff_words->opt->file,
+ fn_out_diff_words_write_helper(diff_words,
&style->old, style->newline,
diff_words->minus.text.size,
diff_words->minus.text.ptr, line_prefix);
@@ -1040,7 +1041,7 @@ static void diff_words_show(struct diff_words_data
*diff_words)
/* special case: only addition */
if (!diff_words->minus.text.size) {
fputs(line_prefix, diff_words->opt->file);
- fn_out_diff_words_write_helper(diff_words->opt->file,
+ fn_out_diff_words_write_helper(diff_words,
&style->new, style->newline,
diff_words->plus.text.size,
diff_words->plus.text.ptr, line_prefix);
@@ -1067,7 +1068,7 @@ static void diff_words_show(struct diff_words_data
*diff_words)
diff_words->plus.text.size) {
if (color_words_output_graph_prefix(diff_words))
fputs(line_prefix, diff_words->opt->file);
- fn_out_diff_words_write_helper(diff_words->opt->file,
+ fn_out_diff_words_write_helper(diff_words,
&style->ctx, style->newline,
diff_words->plus.text.ptr + diff_words->plus.text.size
- diff_words->current_plus, diff_words->current_plus,
--
2.3.0.rc1.137.g477eb31
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/7] diff.c: add new arguments to emit_line_0()
This patch is no-op, to reduce noise in the next one.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
diff.c | 33 ++---
1 file changed, 22 insertions(+), 11 deletions(-)
diff --git a/diff.c b/diff.c
index 3b7317e..47d22e3 100644
--- a/diff.c
+++ b/diff.c
@@ -458,7 +458,9 @@ struct tagged_pointer {
};
static void emit_line_0(struct diff_options *o, const char *set, const char
*reset,
- int first, const char *line, int len)
+ int first, const char *line, int len,
+ struct tagged_pointer *begin,
+ struct tagged_pointer *end)
{
int has_trailing_newline, has_trailing_carriage_return;
int nofirst;
@@ -497,7 +499,7 @@ static void emit_line_0(struct diff_options *o, const char
*set, const char *res
static void emit_line(struct diff_options *o, const char *set, const char
*reset,
const char *line, int len)
{
- emit_line_0(o, set, reset, line[0], line+1, len-1);
+ emit_line_0(o, set, reset, line[0], line+1, len-1, NULL, NULL);
}
static int new_blank_line_at_eof(struct emit_callback *ecbdata, const char
*line, int len)
@@ -516,7 +518,9 @@ static void emit_line_checked(const char *reset,
const char *line, int len,
enum color_diff color,
unsigned ws_error_highlight,
- char sign)
+ char sign,
+ struct tagged_pointer *begin,
+ struct tagged_pointer *end)
{
const char *set = diff_get_color(ecbdata->color_diff, color);
const char *ws = NULL;
@@ -528,13 +532,16 @@ static void emit_line_checked(const char *reset,
}
if (!ws)
- emit_line_0(ecbdata->opt, set, reset, sign, line, len);
+ emit_line_0(ecbdata->opt, set, reset, sign,
+ line, len, begin, end);
else if (sign == '+' && new_blank_line_at_eof(ecbdata, line, len))
/* Blank line at EOF - paint '+' as well */
- emit_line_0(ecbdata->opt, ws, reset, sign, line, len);
+ emit_line_0(ecbdata->opt, ws, reset, sign,
+ line, len, begin, end);
else {
/* Emit just the prefix, then the rest. */
- emit_line_0(ecbdata->opt, set, reset, sign, "", 0);
+ emit_line_0(ecbdata->opt, set, reset, sign,
+ "", 0, NULL, NULL);
ws_check_emit(line, len, ecbdata->ws_rule,
ecbdata->opt->file, set, reset, ws);
}
@@ -545,7 +552,8 @@ static void emit_add_line(const char *reset,
const char *line, int len)
{
emit_line_checked(reset, ecbdata, line, len,
- DIFF_FILE_NEW, WSEH_NEW, '+');
+ DIFF_FILE_NEW, WSEH_NEW, '+',
+ NULL, NULL);
}
static void emit_del_line(const char *reset,
@@ -553,7 +561,8 @@ static void emit_del_line(const char *reset,
const char *line, int len)
{
emit_line_checked(reset, ecbdata, line, len,
- DIFF_FILE_OLD, WSEH_OLD, '-');
+ DIFF_FILE_OLD, WSEH_OLD, '-',
+ NULL, NULL);
}
static void emit_context_line(const char *reset,
@@ -561,7 +570,8 @@ static void emit_context_line(const char *reset,
const char *line, int len)
{
emit_line_checked(reset, ecbdata, line, len,
- DIFF_CONTEXT, WSEH_CONTEXT, ' ');
+ DIFF_CONTEXT, WSEH_CONTEXT, ' ',
+ NULL, NULL);
}
static void emit_hunk_header(struct emit_callback *ecbdata,
@@ -682,7 +692,7 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
DIFF_CONTEXT);
putc('\n', ecb->opt->file);
emit_line_0(ecb->opt, context, reset, '\\',
- nneof, strlen(nneof));
+ nneof, strlen(nneof), NULL, NULL);
}
}
@@ -1301,7 +1311,8 @@ static void diff_words_flush_unified(struct emit_callback
*ecb,
assert(end_line->tag == TAG_END_LINE);
emit_line_checked(reset, ecb, begin_line->str,
end_line->str - begin_line->str,
- color, ws_error_highlight, sign);
+ color, ws_error_highlight, sign,
+ NULL, NULL);
}
b->mark_nr = 0;
}
--
2.3.0.rc1.137.g477eb31
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 08/11] ref-filter: introduce prefixes for the align atom
On Thu, Dec 17, 2015 at 2:29 PM, Eric Sunshine wrote:
> On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak wrote:
>> ref-filter: introduce prefixes for the align atom
>
> The prefixes are actually for the arguments to the 'align' atom, not
> for the atom itself. However, it might be better to describe this at a
> bit higher level. Perhaps:
>
> ref-filter: align: introduce long-form syntax
>
> or something.
Makes sense, thanks.
>
>> Introduce optional prefixes "width=" and "position=" for the align atom
>> so that the atom can be used as "%(align:width=,position=)".
>>
>> Add Documetation and tests for the same.
>>
>> Signed-off-by: Karthik Nayak
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -96,10 +96,19 @@ static void align_atom_parser(struct used_atom *atom)
>>
>> while (*s) {
>> int position;
>> + buf = s[0]->buf;
>
> It probably would be better to do this assignment in the previous
> patch (7/11) since its presence here introduces unwanted noise
> (textual replacement of "s[0]->buf" with "buf") in several locations
> below which slightly obscure the real changes of this patch.
>
This makes sense from a reviewers perspective, will do.
>> - if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
>> + if (skip_prefix(buf, "position=", &buf)) {
>> + position = parse_align_position(buf);
>> + if (position == -1)
>
> It may be more idiomatic in this codebase to detect errors via '< 0'
> rather than explicit '== -1'. Ditto below.
>
I think Junio also mentioned this once. Thanks for reminding.
>> + die(_("unrecognized position:%s"), buf);
>> + align->position = position;
>> + } else if (skip_prefix(buf, "width=", &buf)) {
>> + if (strtoul_ui(buf, 10, (unsigned int *)&width))
>> + die(_("unrecognized width:%s"), buf);
>> + } else if (!strtoul_ui(buf, 10, (unsigned int *)&width))
>> ;
>> - else if ((position = parse_align_position(s[0]->buf)) > 0)
>> + else if ((position = parse_align_position(buf)) != -1)
>> align->position = position;
>> else
>> die(_("unrecognized %%(align) argument: %s"),
>> s[0]->buf);
>
> s/s[0]->//
>
Thanks.
> More below...
>
>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>> @@ -133,6 +133,168 @@ test_expect_success 'right alignment' '
>> test_cmp expect actual
>> '
>>
>> +test_expect_success 'alignment with "position" prefix' '
>> + cat >expect <<-\EOF &&
>> + | refname is refs/heads/master|refs/heads/master
>> + |refname is refs/heads/side|refs/heads/side
>> + | refname is refs/odd/spot|refs/odd/spot
>> + |refname is refs/tags/double-tag|refs/tags/double-tag
>> + | refname is refs/tags/four|refs/tags/four
>> + | refname is refs/tags/one|refs/tags/one
>> + |refname is refs/tags/signed-tag|refs/tags/signed-tag
>> + |refname is refs/tags/three|refs/tags/three
>> + | refname is refs/tags/two|refs/tags/two
>> + EOF
>> + git for-each-ref --format="|%(align:30,position=right)refname is
>> %(refname)%(end)|%(refname)" >actual &&
>> + test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'alignment with "position" prefix' '
>> + cat >expect <<-\EOF &&
>> + | refname is refs/heads/master|refs/heads/master
>> + |refname is refs/heads/side|refs/heads/side
>> + | refname is refs/odd/spot|refs/odd/spot
>> + |refname is refs/tags/double-tag|refs/tags/double-tag
>> + | refname is refs/tags/four|refs/tags/four
>> + | refname is refs/tags/one|refs/tags/one
>> + |refname is refs/tags/signed-tag|refs/tags/signed-tag
>> + |refname is refs/tags/three|refs/tags/three
>> + | refname is refs/tags/two|refs/tags/two
>> + EOF
>> + git for-each-ref --format="|%(align:position=right,30)refname is
>> %(refname)%(end)|%(refname)" >actual &&
>> + test_cmp expect actual
>> +'
>
> This (and below) is a lot of copy/paste code which makes it difficult
> to read the tests and maintain (change) them. Since 'expect' doesn't
> appear to change from test to test, one way to eliminate some of this
> noisiness would be to create 'expect' once outside of the tests.
>
> However, even better, especially from a comprehension,
> maintainability, and extensibility standpoints would be to make this
> all table-driven. In particular, I'd expect to see a table with
> exactly the list of test inputs mentioned in my earlier review[1], and
> have that table passed to a shell function which performs the test for
> each element of the table. For instance, something like:
>
> test_align_permutations <<-\EOF
>
Re: [PATCH v3] reflog-walk: don't segfault on non-commit sha1's in the reflog
On do, 2015-12-31 at 09:57 +0100, Dennis Kaarsemaker wrote: > > > +test_expect_success 'reflog containing non-commit sha1s displays > > > properly' ' > > > > In general, "properly" is a poor word to use in test description > (or > > a commit log message or a bug report, for that matter), as the > whole > > point of a test is to precisely define what is "proper". > > > > And the code change declares that a proper thing to do is to omit > > non-commit entries without segfaulting, so something like > > > > s/displays properly/omits them/ > > > > perhaps? > > I did find the test title a bit iffy but couldn't really figure out > why. What you're saying makes a lot of sense, will fix. Thinking about it a bit more: 'proper' would be to show everything, we just expect that not to work yet. So I'll make it test_expect_failure 'reflog with non-commit entries displays all entries' ' git reflog refs/tests/tree-in-reflog >actual && test_line_count = 3 actual ' -- Dennis Kaarsemaker www.kaarsemaker.net -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ./t3310 fixed, t3400 broken
> t3310-notes-merge-manual-resolve.sh
>failed during a short window (due to commit 2bd811ec) and
> has already been fixed in commit 3a74ea38 ("notes: allow merging
> from arbitrary references", 29-12-2015).
Yes, it's fixed. Sorry for the noise.
The next failure is t3400.
Is there a chance to squeeze in this diff ?
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -259,11 +259,11 @@ test_expect_success 'rebase duplicated commit with
--keep-empty' '
git reset --hard &&
git checkout master &&
- >x && git add x && git commit x -mx &&
- echo x >x && git commit x -mx1 &&
+ >y && git add y && git commit y -my &&
+ echo y >y && git commit y -my1 &&
git checkout -b duplicated HEAD~ &&
- echo x >x && git commit x -mx2 &&
+ echo y >y && git commit y -my2 &&
git rebase --keep-empty master
'
>
> ATB,
> Ramsay Jones
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
GIT GUI - Windows build will open a window on a disconnected display
I use a secondary monitor when at the office and I often will have applications open on the second display. The bug happens when I open the Git GUI application without having the second monitor connected. The dialog window that asks for selection is displayed properly but the main window is lost on another display. You can still get around to the window by using windows keyboard shortcuts but it isn't ideal. This may not be a bug specifically for Git GUI, it may better be reported to WISH or even Microsoft. Windows 7 git-gui version 0.20.GITGUI git version 2.6.3.windows.1 Tcl/Tk version 8.6.4 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
