* [PATCH v4 04/19] builtin/describe: convert to struct object_id
From: brian m. carlson @ 2017-02-20 0:10 UTC (permalink / raw)
To: git; +Cc: Jeff King, Michael Haggerty, Junio C Hamano, Ramsay Jones
In-Reply-To: <20170220001031.559931-1-sandals@crustytoothpaste.net>
Convert the functions in this file and struct commit_name to struct
object_id.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
builtin/describe.c | 50 +++++++++++++++++++++++++-------------------------
1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/builtin/describe.c b/builtin/describe.c
index 01490a157e..738e68f95b 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -39,11 +39,11 @@ static const char *diff_index_args[] = {
struct commit_name {
struct hashmap_entry entry;
- unsigned char peeled[20];
+ struct object_id peeled;
struct tag *tag;
unsigned prio:2; /* annotated tag = 2, tag = 1, head = 0 */
unsigned name_checked:1;
- unsigned char sha1[20];
+ struct object_id oid;
char *path;
};
@@ -54,17 +54,17 @@ static const char *prio_names[] = {
static int commit_name_cmp(const struct commit_name *cn1,
const struct commit_name *cn2, const void *peeled)
{
- return hashcmp(cn1->peeled, peeled ? peeled : cn2->peeled);
+ return oidcmp(&cn1->peeled, peeled ? peeled : &cn2->peeled);
}
-static inline struct commit_name *find_commit_name(const unsigned char *peeled)
+static inline struct commit_name *find_commit_name(const struct object_id *peeled)
{
- return hashmap_get_from_hash(&names, sha1hash(peeled), peeled);
+ return hashmap_get_from_hash(&names, sha1hash(peeled->hash), peeled->hash);
}
static int replace_name(struct commit_name *e,
int prio,
- const unsigned char *sha1,
+ const struct object_id *oid,
struct tag **tag)
{
if (!e || e->prio < prio)
@@ -77,13 +77,13 @@ static int replace_name(struct commit_name *e,
struct tag *t;
if (!e->tag) {
- t = lookup_tag(e->sha1);
+ t = lookup_tag(e->oid.hash);
if (!t || parse_tag(t))
return 1;
e->tag = t;
}
- t = lookup_tag(sha1);
+ t = lookup_tag(oid->hash);
if (!t || parse_tag(t))
return 0;
*tag = t;
@@ -96,24 +96,24 @@ static int replace_name(struct commit_name *e,
}
static void add_to_known_names(const char *path,
- const unsigned char *peeled,
+ const struct object_id *peeled,
int prio,
- const unsigned char *sha1)
+ const struct object_id *oid)
{
struct commit_name *e = find_commit_name(peeled);
struct tag *tag = NULL;
- if (replace_name(e, prio, sha1, &tag)) {
+ if (replace_name(e, prio, oid, &tag)) {
if (!e) {
e = xmalloc(sizeof(struct commit_name));
- hashcpy(e->peeled, peeled);
- hashmap_entry_init(e, sha1hash(peeled));
+ oidcpy(&e->peeled, peeled);
+ hashmap_entry_init(e, sha1hash(peeled->hash));
hashmap_add(&names, e);
e->path = NULL;
}
e->tag = tag;
e->prio = prio;
e->name_checked = 0;
- hashcpy(e->sha1, sha1);
+ oidcpy(&e->oid, oid);
free(e->path);
e->path = xstrdup(path);
}
@@ -154,7 +154,7 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voi
else
prio = 0;
- add_to_known_names(all ? path + 5 : path + 10, peeled.hash, prio, oid->hash);
+ add_to_known_names(all ? path + 5 : path + 10, &peeled, prio, oid);
return 0;
}
@@ -212,7 +212,7 @@ static unsigned long finish_depth_computation(
static void display_name(struct commit_name *n)
{
if (n->prio == 2 && !n->tag) {
- n->tag = lookup_tag(n->sha1);
+ n->tag = lookup_tag(n->oid.hash);
if (!n->tag || parse_tag(n->tag))
die(_("annotated tag %s not available"), n->path);
}
@@ -230,14 +230,14 @@ static void display_name(struct commit_name *n)
printf("%s", n->path);
}
-static void show_suffix(int depth, const unsigned char *sha1)
+static void show_suffix(int depth, const struct object_id *oid)
{
- printf("-%d-g%s", depth, find_unique_abbrev(sha1, abbrev));
+ printf("-%d-g%s", depth, find_unique_abbrev(oid->hash, abbrev));
}
static void describe(const char *arg, int last_one)
{
- unsigned char sha1[20];
+ struct object_id oid;
struct commit *cmit, *gave_up_on = NULL;
struct commit_list *list;
struct commit_name *n;
@@ -246,20 +246,20 @@ static void describe(const char *arg, int last_one)
unsigned long seen_commits = 0;
unsigned int unannotated_cnt = 0;
- if (get_sha1(arg, sha1))
+ if (get_oid(arg, &oid))
die(_("Not a valid object name %s"), arg);
- cmit = lookup_commit_reference(sha1);
+ cmit = lookup_commit_reference(oid.hash);
if (!cmit)
die(_("%s is not a valid '%s' object"), arg, commit_type);
- n = find_commit_name(cmit->object.oid.hash);
+ n = find_commit_name(&cmit->object.oid);
if (n && (tags || all || n->prio == 2)) {
/*
* Exact match to an existing ref.
*/
display_name(n);
if (longformat)
- show_suffix(0, n->tag ? n->tag->tagged->oid.hash : sha1);
+ show_suffix(0, n->tag ? &n->tag->tagged->oid : &oid);
if (dirty)
printf("%s", dirty);
printf("\n");
@@ -276,7 +276,7 @@ static void describe(const char *arg, int last_one)
struct commit *c;
struct commit_name *n = hashmap_iter_first(&names, &iter);
for (; n; n = hashmap_iter_next(&iter)) {
- c = lookup_commit_reference_gently(n->peeled, 1);
+ c = lookup_commit_reference_gently(n->peeled.hash, 1);
if (c)
c->util = n;
}
@@ -380,7 +380,7 @@ static void describe(const char *arg, int last_one)
display_name(all_matches[0].name);
if (abbrev)
- show_suffix(all_matches[0].depth, cmit->object.oid.hash);
+ show_suffix(all_matches[0].depth, &cmit->object.oid);
if (dirty)
printf("%s", dirty);
printf("\n");
--
2.11.0
^ permalink raw reply related
* [PATCH v4 03/19] builtin/diff-tree: convert to struct object_id
From: brian m. carlson @ 2017-02-20 0:10 UTC (permalink / raw)
To: git; +Cc: Jeff King, Michael Haggerty, Junio C Hamano, Ramsay Jones
In-Reply-To: <20170220001031.559931-1-sandals@crustytoothpaste.net>
Convert most leaf functions to struct object_id. Change several
hardcoded numbers to uses of parse_oid_hex. In doing so, verify that we
when we want two trees, we have exactly two trees.
Finally, in stdin_diff_commit, avoid accessing the byte after the NUL.
This will be a NUL as well, since the first NUL was a newline we
overwrote. However, with parse_oid_hex, we no longer need to increment
the pointer directly, and can simply increment it as part of our check
for the space character.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/diff-tree.c | 43 +++++++++++++++++++++----------------------
1 file changed, 21 insertions(+), 22 deletions(-)
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 8ce00480cd..1656e092bd 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -7,46 +7,44 @@
static struct rev_info log_tree_opt;
-static int diff_tree_commit_sha1(const unsigned char *sha1)
+static int diff_tree_commit_sha1(const struct object_id *oid)
{
- struct commit *commit = lookup_commit_reference(sha1);
+ struct commit *commit = lookup_commit_reference(oid->hash);
if (!commit)
return -1;
return log_tree_commit(&log_tree_opt, commit);
}
/* Diff one or more commits. */
-static int stdin_diff_commit(struct commit *commit, char *line, int len)
+static int stdin_diff_commit(struct commit *commit, const char *p)
{
- unsigned char sha1[20];
- if (isspace(line[40]) && !get_sha1_hex(line+41, sha1)) {
+ struct object_id oid;
+ if (isspace(*p++) && !parse_oid_hex(p, &oid, &p)) {
/* Graft the fake parents locally to the commit */
- int pos = 41;
struct commit_list **pptr;
/* Free the real parent list */
free_commit_list(commit->parents);
commit->parents = NULL;
pptr = &(commit->parents);
- while (line[pos] && !get_sha1_hex(line + pos, sha1)) {
- struct commit *parent = lookup_commit(sha1);
+ while (isspace(*p++) && !parse_oid_hex(p, &oid, &p)) {
+ struct commit *parent = lookup_commit(oid.hash);
if (parent) {
pptr = &commit_list_insert(parent, pptr)->next;
}
- pos += 41;
}
}
return log_tree_commit(&log_tree_opt, commit);
}
/* Diff two trees. */
-static int stdin_diff_trees(struct tree *tree1, char *line, int len)
+static int stdin_diff_trees(struct tree *tree1, const char *p)
{
- unsigned char sha1[20];
+ struct object_id oid;
struct tree *tree2;
- if (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1))
+ if (!isspace(*p++) || parse_oid_hex(p, &oid, &p) || *p)
return error("Need exactly two trees, separated by a space");
- tree2 = lookup_tree(sha1);
+ tree2 = lookup_tree(oid.hash);
if (!tree2 || parse_tree(tree2))
return -1;
printf("%s %s\n", oid_to_hex(&tree1->object.oid),
@@ -60,23 +58,24 @@ static int stdin_diff_trees(struct tree *tree1, char *line, int len)
static int diff_tree_stdin(char *line)
{
int len = strlen(line);
- unsigned char sha1[20];
+ struct object_id oid;
struct object *obj;
+ const char *p;
if (!len || line[len-1] != '\n')
return -1;
line[len-1] = 0;
- if (get_sha1_hex(line, sha1))
+ if (parse_oid_hex(line, &oid, &p))
return -1;
- obj = parse_object(sha1);
+ obj = parse_object(oid.hash);
if (!obj)
return -1;
if (obj->type == OBJ_COMMIT)
- return stdin_diff_commit((struct commit *)obj, line, len);
+ return stdin_diff_commit((struct commit *)obj, p);
if (obj->type == OBJ_TREE)
- return stdin_diff_trees((struct tree *)obj, line, len);
+ return stdin_diff_trees((struct tree *)obj, p);
error("Object %s is a %s, not a commit or tree",
- sha1_to_hex(sha1), typename(obj->type));
+ oid_to_hex(&oid), typename(obj->type));
return -1;
}
@@ -141,7 +140,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
break;
case 1:
tree1 = opt->pending.objects[0].item;
- diff_tree_commit_sha1(tree1->oid.hash);
+ diff_tree_commit_sha1(&tree1->oid);
break;
case 2:
tree1 = opt->pending.objects[0].item;
@@ -164,9 +163,9 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
opt->diffopt.setup |= (DIFF_SETUP_USE_SIZE_CACHE |
DIFF_SETUP_USE_CACHE);
while (fgets(line, sizeof(line), stdin)) {
- unsigned char sha1[20];
+ struct object_id oid;
- if (get_sha1_hex(line, sha1)) {
+ if (get_oid_hex(line, &oid)) {
fputs(line, stdout);
fflush(stdout);
}
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v2] git-check-ref-format: clarify man for --normalize
From: Jeff King @ 2017-02-20 0:47 UTC (permalink / raw)
To: Damien Regad; +Cc: git, Philip Oakley, Michael Haggerty
In-Reply-To: <44113ef6-5669-5e02-f848-27c17fac55e5@gmail.com>
On Sun, Feb 19, 2017 at 11:32:32PM +0100, Damien Regad wrote:
> Use of 'iff' may be confusing to people not familiar with this term.
>
> Improving the --normalize option's documentation to remove the use of
> 'iff', and clearly describe what happens when the condition is not met.
Looks good to me. Thanks for following up.
-Peff
^ permalink raw reply
* Re: [RFC PATCH] show decorations at the end of the line
From: Jeff King @ 2017-02-20 0:46 UTC (permalink / raw)
To: Jacob Keller; +Cc: Linus Torvalds, Junio C Hamano, Git Mailing List
In-Reply-To: <CA+P7+xqtPwzt3J6O05TP=E_hh-ko97adn+__Zmc0DNSDqEnEHw@mail.gmail.com>
On Sun, Feb 19, 2017 at 03:03:21PM -0800, Jacob Keller wrote:
> >> I just got bitten by a fallout. I have
> >>
> >> $ git recent --help
> >> `git recent' is aliased to `log --oneline --branches --no-merges \
> >> --source --since=3.weeks'
> >>
> >> but now the branch names are shown at the end, which defeats the
> >> whole point of the alias.
> >
> > Yes, your situation actually wants those decorations as primary
> > things, so having them at the end is indeed pointless.
> >
> > So I think we should just discard that patch of mine.
> >
> > Linus
>
> I would think that in general putting them at the end makes more
> sense, but we should have the ability to use them in format specifiers
> so that users are free to customize it exactly how they want. That is,
> I agree with the reasoning presented in the original patch, but think
> Junio's case can be solved by strengthening the custom formats.
I think there are two potential patches:
1. Add a custom-format placeholder for the --source value.
This is an obvious improvement that doesn't hurt anyone.
2. Switch --decorate to the end by default, but _not_ --source.
This use case _could_ be served already by using a custom format
with "%d". So it's really just a matter of having better-looking
default.
It might hurt somebody's script, but for the reasons discussed
earlier in the thread, people are unlikely to be parsing it (it's
more likely somebody would just complain because they think the
decoration-first behavior is prettier).
-Peff
^ permalink raw reply
* Re: [RFC PATCH] show decorations at the end of the line
From: Junio C Hamano @ 2017-02-20 1:15 UTC (permalink / raw)
To: Jeff King; +Cc: Jacob Keller, Linus Torvalds, Git Mailing List
In-Reply-To: <20170220004648.c2zz6bm2hylvep6x@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I think there are two potential patches:
>
> 1. Add a custom-format placeholder for the --source value.
> This is an obvious improvement that doesn't hurt anyone.
>
> 2. Switch --decorate to the end by default, but _not_ --source.
>
> This use case _could_ be served already by using a custom format
> with "%d". So it's really just a matter of having better-looking
> default.
Yes, and I agree it is a better default to have "--decorate" at the
end.
I do not mind having to use a custom format myself, but I suspect
that the default for "--source" is more useful to have it at the
beginning, because "--source" annotates each and every commit, as
opposed to "--decorate" that adds annotation few and far between.
^ permalink raw reply
* Re: [RFC PATCH] show decorations at the end of the line
From: Linus Torvalds @ 2017-02-20 1:48 UTC (permalink / raw)
To: Jeff King; +Cc: Jacob Keller, Junio C Hamano, Git Mailing List
In-Reply-To: <20170220004648.c2zz6bm2hylvep6x@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 910 bytes --]
On Sun, Feb 19, 2017 at 4:46 PM, Jeff King <peff@peff.net> wrote:
>
> I think there are two potential patches:
>
> 1. Add a custom-format placeholder for the --source value.
> This is an obvious improvement that doesn't hurt anyone.
Right.
> 2. Switch --decorate to the end by default, but _not_ --source.
.. and in fact the whole "--source" printing should not even have been
mixed up with the decorations.
So (2) is actually easy to fix: just don't mix "show_source()" with
"show_decorations()", because they are totally different things to
begin with.
That source showing should never have been in "show_decorations()" in
the first place. It just happened to be a convenient place for it.
So this attached patch is just my original patch updated to split up
"show_source()" from "show_decorations()", and show it where it used
to be.
Maybe this works for Junio's alias?
Linus
[-- Attachment #2: 0001-show-decorations-at-the-end-of-the-line.patch --]
[-- Type: text/x-patch, Size: 6152 bytes --]
From 97abab56fed451476f6ec676346b1e66001ef864 Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sat, 11 Feb 2017 10:03:33 -0800
Subject: [PATCH] show decorations at the end of the line
So I use "--show-decorations" all the time because I find it very useful
to see where the origin branch is, where tags are etc. In fact, my global
git config file has
[log]
decorate = auto
in it, so that I don't have to type it out all the time when I just do my
usual 'git log". It's lovely.
However, it does make one particular case uglier: with commit decorations,
the "oneline" commit format ends up being not very pretty:
[torvalds@i7 git]$ git log --oneline -10
3f07dac29 (HEAD -> master) pathspec: don't error out on all-exclusionary pathspec patterns
ca4a562f2 pathspec magic: add '^' as alias for '!'
02555c1b2 ls-remote: add "--diff" option to show only refs that differ
6e3a7b339 (tag: v2.12.0-rc0, origin/master, origin/HEAD) Git 2.12-rc0
fafca0f72 Merge branch 'cw/log-updates-for-all-refs-really'
74dee5cfa Merge branch 'pl/complete-diff-submodule-diff'
36acf4123 Merge branch 'rs/object-id'
ecc486b1f Merge branch 'js/re-running-failed-tests'
4ba6bb2d1 Merge branch 'sb/submodule-update-initial-runs-custom-script'
5348021c6 Merge branch 'sb/submodule-recursive-absorb'
and note how the decoration comes right after the shortened commit hash,
breaking up the alignment of the messages.
The above doesn't show it with the colorization: I also have
[color]
ui=auto
so on my terminal the decoration is also nicely colorized which makes it
much more obvious, it's not as obvious in this message.
The oneline message handling is already pretty special, this makes it even
more special by putting the decorations at the end of the line:
3f07dac29 pathspec: don't error out on all-exclusionary pathspec patterns (HEAD -> master)
ca4a562f2 pathspec magic: add '^' as alias for '!'
02555c1b2 ls-remote: add "--diff" option to show only refs that differ
6e3a7b339 Git 2.12-rc0 (tag: v2.12.0-rc0, origin/master, origin/HEAD)
fafca0f72 Merge branch 'cw/log-updates-for-all-refs-really'
74dee5cfa Merge branch 'pl/complete-diff-submodule-diff'
36acf4123 Merge branch 'rs/object-id'
ecc486b1f Merge branch 'js/re-running-failed-tests'
4ba6bb2d1 Merge branch 'sb/submodule-update-initial-runs-custom-script'
5348021c6 Merge branch 'sb/submodule-recursive-absorb'
which looks a lot better (again, this is all particularly noticeable with
colorization).
NOTE! There's a very special case for "git log --oneline -g" that shows
the reflogs as oneliners, and this does *not* fix that special case. It's
a lot more involved and relies on the exact show_reflog_message()
implementation, so I left the format for that alone, along with a comment
about how it's not at the end of line.
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
builtin/rev-list.c | 1 +
log-tree.c | 17 ++++++++++++++---
log-tree.h | 1 +
3 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 0aa93d589..8833f029a 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -107,6 +107,7 @@ static void show_commit(struct commit *commit, void *data)
children = children->next;
}
}
+ show_source(revs, commit);
show_decorations(revs, commit);
if (revs->commit_format == CMIT_FMT_ONELINE)
putchar(' ');
diff --git a/log-tree.c b/log-tree.c
index 8c2415747..9ca3e8c1c 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -279,12 +279,16 @@ void format_decorations_extended(struct strbuf *sb,
strbuf_addstr(sb, color_reset);
}
+void show_source(struct rev_info *opt, struct commit *commit)
+{
+ if (opt->show_source && commit->util)
+ fprintf(opt->diffopt.file, "\t%s", (char *) commit->util);
+}
+
void show_decorations(struct rev_info *opt, struct commit *commit)
{
struct strbuf sb = STRBUF_INIT;
- if (opt->show_source && commit->util)
- fprintf(opt->diffopt.file, "\t%s", (char *) commit->util);
if (!opt->show_decorations)
return;
format_decorations(&sb, commit, opt->diffopt.use_color);
@@ -556,6 +560,7 @@ void show_log(struct rev_info *opt)
show_parents(commit, abbrev_commit, opt->diffopt.file);
if (opt->children.name)
show_children(opt, commit, abbrev_commit);
+ show_source(opt, commit);
show_decorations(opt, commit);
if (opt->graph && !graph_is_commit_finished(opt->graph)) {
putc('\n', opt->diffopt.file);
@@ -622,10 +627,14 @@ void show_log(struct rev_info *opt)
find_unique_abbrev(parent->object.oid.hash,
abbrev_commit));
fputs(diff_get_color_opt(&opt->diffopt, DIFF_RESET), opt->diffopt.file);
- show_decorations(opt, commit);
+ show_source(opt, commit);
if (opt->commit_format == CMIT_FMT_ONELINE) {
+ /* Not at end of line, but.. */
+ if (opt->reflog_info)
+ show_decorations(opt, commit);
putc(' ', opt->diffopt.file);
} else {
+ show_decorations(opt, commit);
putc('\n', opt->diffopt.file);
graph_show_oneline(opt->graph);
}
@@ -716,6 +725,8 @@ void show_log(struct rev_info *opt)
opt->missing_newline = 0;
graph_show_commit_msg(opt->graph, opt->diffopt.file, &msgbuf);
+ if (ctx.fmt == CMIT_FMT_ONELINE)
+ show_decorations(opt, commit);
if (opt->use_terminator && !commit_format_is_empty(opt->commit_format)) {
if (!opt->missing_newline)
graph_show_padding(opt->graph);
diff --git a/log-tree.h b/log-tree.h
index c8116e60c..33d415820 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -20,6 +20,7 @@ void format_decorations_extended(struct strbuf *sb, const struct commit *commit,
const char *suffix);
#define format_decorations(strbuf, commit, color) \
format_decorations_extended((strbuf), (commit), (color), " (", ", ", ")")
+void show_source(struct rev_info *opt, struct commit *commit);
void show_decorations(struct rev_info *opt, struct commit *commit);
void log_write_email_headers(struct rev_info *opt, struct commit *commit,
const char **subject_p,
--
2.12.0.rc2.4.g97abab56f
^ permalink raw reply related
* Re: Git bisect does not find commit introducing the bug
From: Oleg Taranenko @ 2017-02-20 7:38 UTC (permalink / raw)
To: Jacob Keller
Cc: Alex Hoffman, Johannes Sixt, Christian Couder, Stephan Beyer, git
In-Reply-To: <CA+P7+xrch9WDo6OgU3vUEpXqAETZ07mkf76dC9nJctm0LTFQHQ@mail.gmail.com>
>>> Then you must adjust your definition of "good": All commits that do not have
>>> the feature, yet, are "good": since they do not have the feature in the
>>> first place, they cannot have the breakage that you found in the feature.
>>>
>>> That is exactly the situation in your original example! But you constructed
>>> the condition of goodness in such a simplistic way (depending on the
>>> presence of a string), that it was impossible to distinguish between "does
>>> not have the feature at all" and "has the feature, but it is broken".
>>
>> Johannes, thank you for correctly identifying the error in my logic.
>> Indeed I was using the term 'bad' also for the commit without the
>> feature. In order to find the commit introducing the bug in my example
>> a new state is needed, which would make 'git bisect' a bit more
>> complicated than the user 'most of the time' probably needs. Or do you
>> think, it would make sense to ask the user for this state (if e.g 'git
>> bisect' would be started with a new parameter)?
> If a commit doesn't have the feature, then it is by definition, not
> containing a broken feature, and you can simply use the "good" state.
> There is no need for a different state. If you can't test the commit
> because it's broken in some other way, you can use "git bisect skip"
> but that isn't what you want in this case.
Commits missing feature == 'good' commit is a very confusing one.
Looks like in real life it happens much often, then git developers can
imagine. For multi-branch/multi-feature workflow it's pretty easy not
to recognize whether it is missing or not developed yet, especially on
retrospective view where cherry-picking/squashing/merging is being
used. My experience shows most annoying bugs are generating after a
heavy merge (evil merge) with conflicts resolutions, where developer
is not involved in the knowing what happens on counterpart changes.
Then feature can be disappeared after it was worked & tested in its
own branches.
@Alex, I'm pretty interesting in fixing this weird bisect behaviour as
well, as far as I struggled on it last summer and continue struggling
so far :) If you want we can join to your efforts on fixing.
Cheers, Oleg
^ permalink raw reply
* Re: git-scm.com status report
From: Jeff King @ 2017-02-20 7:53 UTC (permalink / raw)
To: pedro rijo; +Cc: Samuel Lijin, e, Git Users
In-Reply-To: <CAPMsMoDpAeD0hpPuLeWO2T1VoEZDf_hD2gA2GDBqypMF9V6gAw@mail.gmail.com>
On Sat, Feb 18, 2017 at 10:27:51PM +0000, pedro rijo wrote:
> I would say everyone did an amazing job, closing more than 150 old issues
> in a single week! I think the amount of issues is finally manageable (40
> issues currently).
Yes, thank you to all who have been helping. But especially you and
Samuel, who obviously spent a lot of time sifting through old issues.
> And if you agree, I would like to start looking at old PRs (some will
> probably don't make sense anymore), and will start reviewing them as soon
> as I have the time to setup the RoR app on my machine so that I can
> understand and see the changes introduced on the PRs.
Sounds good.
> Many PRs seem to introduce small and innocent changes, but I always like
> to run the code to see :)
Yeah, many of the display-oriented changes are pretty obvious from
reading the code, but I have caught a couple of regressions just by
running the PRs and making sure the rendered result is sane.
-Peff
^ permalink raw reply
* Re: [PATCH v6 0/6] stash: support pathspec argument
From: Jeff King @ 2017-02-20 7:57 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Junio C Hamano, Johannes Schindelin, Øyvind A. Holm,
Jakub Narębski, Matthieu Moy
In-Reply-To: <20170219110313.24070-1-t.gummerer@gmail.com>
On Sun, Feb 19, 2017 at 11:03:07AM +0000, Thomas Gummerer wrote:
> Thanks Junio and Peff for comments on the last round.
>
> Changes since then:
>
> - removed mention of the "new form" of git stash create from the
> Documentation.
> - Changed documentation for git stash without a verb, mentioning
> stash -p now being an alias for git stash push -p and that -- can be
> used as disambiguation for for pathspecs
> - Fixed ${1-...} which should have been ${1?...}
> - Removed unused new_style variable from create_stash, which was a
> leftover from perious rounds.
I gave it one more read-through, and didn't see anything objectionable.
Thanks for seeing this through!
-Peff
^ permalink raw reply
* Re: missing handling of "No newline at end of file" in git am
From: Eric Wong @ 2017-02-20 8:06 UTC (permalink / raw)
To: Olaf Hering; +Cc: Junio C Hamano, git
In-Reply-To: <20170215114430.GD16249@aepfle.de>
Olaf Hering <olaf@aepfle.de> wrote:
> On Tue, Feb 14, Olaf Hering wrote:
>
> > How would I debug it?
>
> One line is supposed to be longer than 998 chars, but something along
> the way truncated it and corrupted the patch.
998 sounds like the SMTP limit.
Perhaps git format-patch should emit binary diffs in that case?
I doubt any human would bother reading excessively long lines as
text...
^ permalink raw reply
* Re: [PATCH v4 03/19] builtin/diff-tree: convert to struct object_id
From: Jeff King @ 2017-02-20 8:09 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Michael Haggerty, Junio C Hamano, Ramsay Jones
In-Reply-To: <20170220001031.559931-4-sandals@crustytoothpaste.net>
On Mon, Feb 20, 2017 at 12:10:15AM +0000, brian m. carlson wrote:
> /* Diff one or more commits. */
> -static int stdin_diff_commit(struct commit *commit, char *line, int len)
> +static int stdin_diff_commit(struct commit *commit, const char *p)
> {
> - unsigned char sha1[20];
> - if (isspace(line[40]) && !get_sha1_hex(line+41, sha1)) {
> + struct object_id oid;
> + if (isspace(*p++) && !parse_oid_hex(p, &oid, &p)) {
> /* Graft the fake parents locally to the commit */
> - int pos = 41;
> struct commit_list **pptr;
>
> /* Free the real parent list */
> free_commit_list(commit->parents);
> commit->parents = NULL;
> pptr = &(commit->parents);
> - while (line[pos] && !get_sha1_hex(line + pos, sha1)) {
> - struct commit *parent = lookup_commit(sha1);
> + while (isspace(*p++) && !parse_oid_hex(p, &oid, &p)) {
> + struct commit *parent = lookup_commit(oid.hash);
> if (parent) {
> pptr = &commit_list_insert(parent, pptr)->next;
> }
> - pos += 41;
> }
> }
Are you sure this is right? The first "if" will advance the "p" pointer,
and we'll miss it in the inner loop.
IOW, the original looked something like:
1. see if we have any parents after the initial commit sha1
2. if so, then free the original parent list, so we can parse the new
ones
3. starting at pos 41 (the same one we parsed in the conditional!),
loop and parse each parent sha1
The conditional in step 1 can't advance our pointer, or we miss the
first parent in step 3.
It's silly to parse the same sha1 twice, though. You could solve it by
adding the first "oid" from the conditional to the new parent list. In
my "something like this" patch, I solved it by dropping the conditional,
and just having the inner loop. It lazily drops the old parent list on
the first iteration.
It's a little disturbing that we do not seem to have even a basic test
of:
git rev-list --parents HEAD | git diff-tree --stdin
which would exercise this code.
-Peff
^ permalink raw reply
* Re: [PATCH v6 0/6] stash: support pathspec argument
From: Junio C Hamano @ 2017-02-20 8:22 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Jeff King, Johannes Schindelin, Øyvind A Holm,
Jakub Narębski, Matthieu Moy
In-Reply-To: <20170219110313.24070-1-t.gummerer@gmail.com>
Thomas Gummerer <t.gummerer@gmail.com> writes:
> @@ -55,10 +53,13 @@ push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q
>
> Save your local modifications to a new 'stash', and run `git reset
> --hard` to revert them. The <message> part is optional and gives
I didn't notice this during v5 review, but the above seems to be
based on the codebase before your documentation update (which used
to be part of the series in older iteration). I had to tweak the
series to apply on top of your 20a7e06172 ("Documentation/stash:
remove mention of git reset --hard", 2017-02-12) while queuing, so
please double check the result when it is pushed out to 'pu'.
Thanks.
^ permalink raw reply
* Re: url.<base>.insteadOf vs. submodules
From: Jeff King @ 2017-02-20 9:01 UTC (permalink / raw)
To: Toolforger; +Cc: git
In-Reply-To: <84fcb0bd-85dc-0142-dd58-47a04eaa7c2b@durchholz.org>
On Sun, Feb 19, 2017 at 10:12:28PM +0100, Toolforger wrote:
> I am trying to make url.<base>.insteadOf work on the URLs inside
> .gitmodules, but it won't work (applying it to the repo itself works fine,
> to the config setting seems to be fine).
The submodule operations happen in their own processes, and do not look
at the config of the parent repo. Are you setting the config in
.git/config of the super-project?
I don't know if there plans to make that work, but one workaround is to
set the config in ~/.gitconfig.
-Peff
^ permalink raw reply
* Re: [PATCH v2] git-check-ref-format: clarify man for --normalize
From: Junio C Hamano @ 2017-02-20 9:10 UTC (permalink / raw)
To: Jeff King; +Cc: Damien Regad, git, Philip Oakley, Michael Haggerty
In-Reply-To: <20170220004717.7nqwk62vnuqv2rbb@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sun, Feb 19, 2017 at 11:32:32PM +0100, Damien Regad wrote:
>
>> Use of 'iff' may be confusing to people not familiar with this term.
>>
>> Improving the --normalize option's documentation to remove the use of
>> 'iff', and clearly describe what happens when the condition is not met.
>
> Looks good to me. Thanks for following up.
Looks good except that there does not seem to be sign-off.
Damien, no need to resend the patch but I need to hear you say that
you have read Documentation/SubmittingPatches and you want your
sign-off added.
Thanks.
^ permalink raw reply
* Re: Git bisect does not find commit introducing the bug
From: Junio C Hamano @ 2017-02-20 9:02 UTC (permalink / raw)
To: Christian Couder; +Cc: Alex Hoffman, Johannes Sixt, Stephan Beyer, git
In-Reply-To: <CAP8UFD3ngMvVy2XLzYNn9OFbS+zQpWTW=pravpHhA-0PcDVhfg@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> git bisect makes some assumptions that are true most of the time, so
> in practice it works well most of the time.
I think we need to clarify the documentation and ask you to stop
using such fuzzy phrases like "assumptions" and "most of the time"
;-).
For bisection to work, "git bisect" requires a very simple thing,
which is that your history is bisectable wrt one particular "trait",
i.e. what you are interested in (e.g. the trait may be "the commit
has this feature in a broken form").
What does it mean for a history to be "bisectable", then?
A bisectable history has commits with the "trait" and commits
without the "trait". Given any commit with the "trait", all commits
that are decendant of such commit must have the "trait". Also given
any commit without the "trait", all commits that are ancestor of
such a commit must not have the "trait".
And your goal is to find one commit with the "trait" whose direct
parent commits all lack the "trait".
If and only if you can formulate your problem into the above form,
"git bisect" can help you by not requiring you to check each and
every commit in the history.
Depending on the way you define the "trait", your history may not be
bisectable, but by formulating the "trait" carefully, many such
history can be made bisectable. In the "Recently some commit broke
feature X. Before then the feature used to work. In an ancient
past, the feature did not even exist" example, if you set "trying to
use feature X breaks" as the "trait", your history is not bisectable
unless you ignore the ancient part that did not even have the
feature. If you restate your "trait" to "feature X does not exist
in a broken form", however, the history becomes bisectable.
Historically, we called commits with "trait" BAD and others GOOD,
because bisection was used primarily to hunt for bugs. It may be
easier to understand if the user thinks of commits without "trait"
as OLD (i.e. commits in the older part of the history are not yet
contaminated), and commits with "trait" as NEW (i.e. at some point,
there is an event to introduce the "trait" and newer part of the
history is contaminated by that event ever since).
^ permalink raw reply
* Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line
From: Junio C Hamano @ 2017-02-20 9:58 UTC (permalink / raw)
To: Jeff King; +Cc: Lars Schneider, Jonathan Tan, git, sbeller
In-Reply-To: <xmqqwpcptxps.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> I still haven't queued any of the variants I posted (and I do not
> think other people sent their own versions, either). I need to pick
> one and queue, with a test or two. Perhaps after -rc2.
>
> Others are welcome to work on it while I cut -rc2 tomorrow, so that
> by the time I see their patch all that is left for me to do is to
> apply it ;-)
Since nothing seems to have happened in the meantime, here is what
I'll queue so that we won't forget for now. Lars's tests based on
how the scripted "git submodule" uses "git config" may still be
valid, but it is somewhat a roundabout way to demonstrate the
breakage and not very effective way to protect the fix, so I added a
new test that directly tests "git -c <var>=<val> <command>".
I am not sure if this updated one is worth doing, or the previous
"strchr and strrchr" I originally wrote was easier to understand.
One thing I noticed is that "git config --get X" will correctly
diagnose that a dot-less X is not a valid variable name, but we do
not seem to diagnose "git -c X=V <cmd>" as invalid.
Perhaps we should, but it is not the focus of this topic.
-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 15 Feb 2017 15:48:44 -0800
Subject: [PATCH] config: preserve <subsection> case for one-shot config on the
command line
The "git -c <var>=<val> cmd" mechanism is to pretend that a
configuration variable <var> is set to <val> while the cmd is
running. The code to do so however downcased <var> in its entirety,
which is wrong for a three-level <section>.<subsection>.<variable>.
The <subsection> part needs to stay as-is.
Reported-by: Lars Schneider <larsxschneider@gmail.com>
Diagnosed-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
config.c | 30 ++++++++++++++++++++++++++++-
t/t1351-config-cmdline.sh | 48 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 77 insertions(+), 1 deletion(-)
create mode 100755 t/t1351-config-cmdline.sh
diff --git a/config.c b/config.c
index 0dfed682b8..ba9a5911b0 100644
--- a/config.c
+++ b/config.c
@@ -199,6 +199,34 @@ void git_config_push_parameter(const char *text)
strbuf_release(&env);
}
+/*
+ * downcase the <section> and <variable> in <section>.<variable> or
+ * <section>.<subsection>.<variable> and do so in place. <subsection>
+ * is left intact.
+ */
+static void canonicalize_config_variable_name(char *varname)
+{
+ char *cp, *last_dot;
+
+ /* downcase the first segment */
+ for (cp = varname; *cp; cp++) {
+ if (*cp == '.')
+ break;
+ *cp = tolower(*cp);
+ }
+ if (!*cp)
+ return;
+
+ /* scan for the last dot */
+ for (last_dot = cp; *cp; cp++)
+ if (*cp == '.')
+ last_dot = cp;
+
+ /* downcase the last segment */
+ for (cp = last_dot; *cp; cp++)
+ *cp = tolower(*cp);
+}
+
int git_config_parse_parameter(const char *text,
config_fn_t fn, void *data)
{
@@ -221,7 +249,7 @@ int git_config_parse_parameter(const char *text,
strbuf_list_free(pair);
return error("bogus config parameter: %s", text);
}
- strbuf_tolower(pair[0]);
+ canonicalize_config_variable_name(pair[0]->buf);
if (fn(pair[0]->buf, value, data) < 0) {
strbuf_list_free(pair);
return -1;
diff --git a/t/t1351-config-cmdline.sh b/t/t1351-config-cmdline.sh
new file mode 100755
index 0000000000..acb8dc3b15
--- /dev/null
+++ b/t/t1351-config-cmdline.sh
@@ -0,0 +1,48 @@
+#!/bin/sh
+
+test_description='git -c var=val'
+
+. ./test-lib.sh
+
+test_expect_success 'last one wins: two level vars' '
+ echo VAL >expect &&
+
+ # sec.var and sec.VAR are the same variable, as the first
+ # and the last level of a configuration variable name is
+ # case insensitive. Test both setting and querying.
+
+ git -c sec.var=val -c sec.VAR=VAL config --get sec.var >actual &&
+ test_cmp expect actual &&
+ git -c SEC.var=val -c sec.var=VAL config --get sec.var >actual &&
+ test_cmp expect actual &&
+
+ git -c sec.var=val -c sec.VAR=VAL config --get SEC.var >actual &&
+ test_cmp expect actual &&
+ git -c SEC.var=val -c sec.var=VAL config --get sec.VAR >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'last one wins: three level vars' '
+ echo val >expect &&
+
+ # v.a.r and v.A.r are not the same variable, as the middle
+ # level of a three-level configuration variable name is
+ # case sensitive. Test both setting and querying.
+
+ git -c v.a.r=val -c v.A.r=VAL config --get v.a.r >actual &&
+ test_cmp expect actual &&
+ git -c v.a.r=val -c v.A.r=VAL config --get V.a.R >actual &&
+ test_cmp expect actual &&
+
+ echo VAL >expect &&
+ git -c v.a.r=val -c v.a.R=VAL config --get v.a.r >actual &&
+ test_cmp expect actual &&
+ git -c v.a.r=val -c V.a.r=VAL config --get v.a.r >actual &&
+ test_cmp expect actual &&
+ git -c v.a.r=val -c v.a.R=VAL config --get V.a.R >actual &&
+ test_cmp expect actual &&
+ git -c v.a.r=val -c V.a.r=VAL config --get V.a.R >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
2.12.0-rc2-221-g8fa194a99f
^ permalink raw reply related
* Re: Cross-referencing the Git mailing list archive with their corresponding commits in `pu`
From: Johannes Schindelin @ 2017-02-17 17:50 UTC (permalink / raw)
To: Josh Triplett; +Cc: git
In-Reply-To: <alpine.DEB.2.20.1702041206130.3496@virtualbox>
Hi Josh,
On Mon, 6 Feb 2017, Johannes Schindelin wrote:
> as discussed at the GitMerge, I am trying to come up with tooling that
> will allow for substantially less tedious navigation between the local
> repository, the mailing list, and what ends up in the `pu` branch.
I found a little bit more time last Friday to play with the
cross-correlation between commits in `pu` and mails in
public-inbox/git.git and it is worse than I previously assumed.
Just as a reminder: my plan was to start developing tools that will
ultimately help me as well as other contributors with the arcane mailing
list model of patch submission. And my first target was the seemingly
simple task of figuring out the mail corresponding to any given commit in
`pu` (i.e. the mail that contained the patch, and whose mail thread is
hence expected to have the entire patch review, and to which I would be
expected to respond if I find a problem with that commit).
And since it is all-too-common that the oneline is adjusted before
applying the patch, the Subject:/oneline pair is not a good candidate to
find matches.
My next best guess was that the author date would not be touched, so the
pair of Date: and authordate should make a good candidate.
My initial finding was that this is not without problems, as some mails
were sent with identical Date: lines (most likely due to bugs in the
tools, e.g. the well-known and already fixed bug in git-am, and hence
git-rebase, where it would apply all patches using the first patch's
author date), and worse: some of those mails contained actual patch series
that actually made it into Git's commit history.
But those are not the only problems.
For starters, I tried to cross-correlate *just* the commits that entered
`pu` since one week ago (git rev-list --since=1.week.ago upstream/pu) with
mails of the past month in the mailing list archive.
One obvious caveat is that RFC 2822 is ambiguous when it comes to the date
format. While it seems nice that you *can* write single-digit day numbers
as single digit if you want, or with a leading zero, or with a leading
space, it makes it impossible to get away with exact matching. I did not
really want to complicate my research by parsing the dates and normalizing
them to epoch + timezone, also because I wanted results quick, so I simply
normalized the dates to have leading zeroes for single-digit day numbers,
that seems to work for the moment).
The first category of problematic commits come as no surprise: merges. We
do not even have a way to represent them as mails. I simply excluded them
from the remainder of this study.
The second category should not be all that surprising, too: Junio often
adjusts the release notes without sending those patches out for review.
Those commits are:
363588f (### match next, Junio C Hamano 2017-02-17)
2076907 (Git 2.12-rc2, Junio C Hamano 2017-02-17)
076c053 (Hopefully the final batch of mini-topics before the final,
Junio C Hamano 2017-02-16)
ae86372 (Revert "reset: add an example of how to split a commit into two",
Junio C Hamano 2017-02-16)
d09b692 (A bit more for -rc2, Junio C Hamano 2017-02-15)
There is a third category, and this one *does* come as a surprise to me.
It appears that at least *some* patches' Date: lines are either ignored or
overridden or changed on their way from the mailing list into Git's commit
history. There was only one commit in that commit range:
3c0cb0c (read_loose_refs(): read refs using resolve_ref_recursively(),
Michael Haggerty 2017-02-09)
This one was committed with an author date "Thu, 09 Feb 2017 21:53:52
+0100" but it appears that there was no mail sent to the Git mailing list
with that particular Date: header and the *actual* mail containing the
patch was sent with a Date: header "Fri, 10 Feb 2017 12:16:19 +0100"
(Message-ID:
d8e906d969700acbca8dc717673d0a9cdc910f62.1486724698.git.mhagger@alum.mit.edu).
It is labor-intensive, but possible to find the correlation manually in
this case because the Subject: line has been left intact.
However, this points to a serious problem with my approach: I try to
re-create information that is actually not available (which Message-ID
corresponds to a given commit name). Since that information is not
available, it is quite possible that this information cannot be retrieved
accurately (and Michael's commit demonstrates that this is not a merely
theoretic consideration). I do not know that I can fix this on my side.
> P.S.: I used public-inbox.org links instead of commit references to the
> Git repository containing the mailing list archive, because the format
> of said Git repository is so unfavorable that it was determined very
> quickly in a discussion between Patrick Reynolds (GitHub) and myself
> that it would put totally undue burden on GitHub to mirror it there
> (compare also Carlos Nieto's talk at GitMerge titled "Top Ten Worst
> Repositories to host on GitHub").
Since the main problem was the unfavorable commit history structure, I
*think* that it may be possible to auto-process public-inbox.org/git.git
into a frequently-rewritten branch that squashes all commits from past
years into single, per-year commits (and the same for recent months, the
past days, and a single commit accumulating the current day's commits) and
that that may solve the problematic structure. The blob names would remain
identical to what is on public-inbox, of course.
Ciao,
Johannes
P.S.: The *mini* scripts I used were
cat generate-date-index.sh <<\EOF
#! /bin/sh
cd public-inbox-git
since_commit="$1"
test -n "$since_commit" ||
since_commit=$(git rev-list --since=1.month.ago master --reverse | head -n 1)
for sha1 in $(git diff --raw --no-abbrev $since_commit..master | cut -f 4 -d \ )
do
printf '%s\t%s\n' \
"$(git cat-file blob $sha1 |
sed -n \
-e 's/^Date:[ ]*\([^,]*,\) *\([1-9] .*\)/\1 0\2/p' \
-e 's/^Date:[ ]*\([^,]*,\) *\([0-9][0-9] .*\)/\1 \2/p' \
-e '/^$/q')" \
$sha1
done | less -S
EOF
to generate a file date-index.txt containing "date\tblob" pairs where the blob
refers to the SHA-1 of the mail in public-inbox/git.git, and
cat >match-pu.sh <<\EOF
#! /bin/sh
for commit in $(git rev-list --since=1.week.ago --no-merges upstream/pu)
do
date="$(git show -s --format=%aD $commit |
sed 's/, \([1-9]\) /, 0\1 /')" # fix up Git's idea of RFC 2822
mail_id=$(grep "^$date" date-index.txt | sed 's/.* //')
case "$mail_id" in
'')
echo "ERROR: no mail found for $commit (date $date)" >&2
git show -s --pretty='tformat:%h (%s, %an %ad)' --date=short \
$commit >&2
;;
*' '*)
echo "ERROR: multiple candidates found for $commit ($mail_id)" >&2
;;
*)
echo "$date $mail_id"
;;
esac
done
EOF
to try to match the author dates with the ones in date-index.txt. The
obvious next improvement is to list also Message-ID in date-index.txt.
^ permalink raw reply
* Re: [PATCH v2 04/16] files-backend: replace *git_path*() with files_path()
From: Michael Haggerty @ 2017-02-20 11:23 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170216114818.6080-5-pclouds@gmail.com>
On 02/16/2017 12:48 PM, Nguyễn Thái Ngọc Duy wrote:
> This centralizes all path rewriting of files-backend.c in one place so
> we have easier time removing the path rewriting later. There could be
> some hidden indirect git_path() though, I didn't audit the code to the
> bottom.
Almost all of the calls to `files_path()` [1] take one of the following
forms:
* `files_path(refs, &sb, "packed-refs")`
* `files_path(refs, &sb, "%s", refname)`
* `files_path(refs, &sb, "logs/%s", refname)`
(though sometimes `refname` is not the name of a reference but rather
the name of a directory containing references, like "refs/heads").
This suggests to me that it would be more natural to have three
functions, `files_packed_refs_path()`, `files_loose_ref_path()`, and
`files_reflog_path()`, with no `fmt` arguments. Aside from making the
callers a bit simpler and the implementation of each of the three
functions simpler (they wouldn't have to deal with variable argument
lists), at the cost of needing three similar functions.
But I think the split would also be advantageous from a design point of
view. The relative path locations of loose reference files, reflog
files, and the packed-refs file is kind of a coincidence, and it would
be advantageous to encode that policy in three functions rather than
continuing to spread knowledge of those assumptions around.
It would also make it easier to switch to a new system of encoding
reference names, for example so that reference names that differ only in
case can be stored on a case-insensitive filesystem, or to store reflogs
using a naming scheme that is not susceptible to D/F conflicts so that
we can retain reflogs for deleted references.
Michael
[1] The only exception I see is one call `files_path(refs, &sb,
"logs")`, which is a prelude to iterating over the reflog files. This is
actually an example of the code giving us a hint that the design is
wrong: if you iterate only over the directories under `git_path(logs)`,
you miss the reflogs for worktree references.
^ permalink raw reply
* Re: [PATCH v4 06/15] files-backend: remove the use of git_path()
From: Michael Haggerty @ 2017-02-20 11:34 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170218133303.3682-7-pclouds@gmail.com>
On 02/18/2017 02:32 PM, Nguyễn Thái Ngọc Duy wrote:
> Given $GIT_DIR and $GIT_COMMON_DIR, files-backend is now in charge of
> deciding what goes where. The end goal is to pass $GIT_DIR only. A
> refs "view" of a linked worktree is a logical ref store that combines
> two files backends together.
>
> (*) Not entirely true since strbuf_git_path_submodule() still does path
> translation underneath. But that's for another patch.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> refs/files-backend.c | 37 +++++++++++++++++++++++++++++++++----
> 1 file changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index b599ddf92..dbcaf9bda 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -924,6 +924,9 @@ struct files_ref_store {
> */
> const char *submodule;
>
> + struct strbuf gitdir;
> + struct strbuf gitcommondir;
Is there a reason for these to be `strbuf`s rather than `const char *`?
(One reason would be if you planned to use the `len` field, but I don't
think you do so.)
> @@ -937,15 +940,33 @@ static void files_path(struct files_ref_store *refs, struct strbuf *sb,
> {
> struct strbuf tmp = STRBUF_INIT;
> va_list vap;
> + const char *ref;
>
> va_start(vap, fmt);
> strbuf_vaddf(&tmp, fmt, vap);
> va_end(vap);
> - if (refs->submodule)
> + if (refs->submodule) {
> strbuf_git_path_submodule(sb, refs->submodule,
> "%s", tmp.buf);
> - else
> - strbuf_git_path(sb, "%s", tmp.buf);
> + } else if (!strcmp(tmp.buf, "packed-refs") ||
> + !strcmp(tmp.buf, "logs")) { /* non refname path */
> + strbuf_addf(sb, "%s/%s", refs->gitcommondir.buf, tmp.buf);
> + } else if (skip_prefix(tmp.buf, "logs/", &ref)) { /* reflog */
> + if (is_per_worktree_ref(ref))
> + strbuf_addf(sb, "%s/%s", refs->gitdir.buf, tmp.buf);
> + else
> + strbuf_addf(sb, "%s/%s", refs->gitcommondir.buf, tmp.buf);
This code would also be simpler if there were separate functions for
packed-refs, loose references, and reflogs.
> [...]
Michael
^ permalink raw reply
* [PATCH v2] send-email: only allow one address per body tag
From: Johan Hovold @ 2017-02-20 11:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: Matthieu Moy, Jeff King, Kevin Daudt, Larry Finger, git,
Johan Hovold
In-Reply-To: <xmqqbmu0pgg6.fsf@gitster.mtv.corp.google.com>
Adding comments after a tag in the body is a common practise (e.g. in
the Linux kernel) and git-send-email has been supporting this for years
by removing any trailing cruft after the address.
After some recent changes, any trailing comment is now instead appended
to the recipient name (with some random white space inserted) resulting
in undesirable noise in the headers, for example:
CC: "# 3 . 3 . x : 1b9508f : sched : Rate-limit newidle" <stable@vger.kernel.org>
Revert to the earlier behaviour of discarding anything after the (first)
address in a tag while parsing the body.
Note that multiple addresses after are still allowed after a command
line switch (and in a CC header field).
Also note that --suppress-cc=self was never honoured when using multiple
addresses in a tag.
Fixes: b1c8a11c8024 ("send-email: allow multiple emails using --cc, --to
and --bcc")
Fixes: e3fdbcc8e164 ("parse_mailboxes: accept extra text after <...>
address")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
v2:
- update the cc-trailer test
- amend commit message and mention the broken --suppress-cc=self
git-send-email.perl | 2 +-
t/t9001-send-email.sh | 7 +++----
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 068d60b3e698..eea0a517f71b 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1563,7 +1563,7 @@ foreach my $t (@files) {
# Now parse the message body
while(<$fh>) {
$message .= $_;
- if (/^(Signed-off-by|Cc): (.*)$/i) {
+ if (/^(Signed-off-by|Cc): ([^>]*>?)/i) {
chomp;
my ($what, $c) = ($1, $2);
chomp $c;
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 0f398dd1603d..60a80f60b268 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -148,7 +148,6 @@ cat >expected-cc <<\EOF
!two@example.com!
!three@example.com!
!four@example.com!
-!five@example.com!
EOF
"
@@ -159,9 +158,9 @@ test_expect_success $PREREQ 'cc trailer with various syntax' '
Test Cc: trailers.
Cc: one@example.com
- Cc: <two@example.com> # this is part of the name
- Cc: <three@example.com>, <four@example.com> # not.five@example.com
- Cc: "Some # Body" <five@example.com> [part.of.name.too]
+ Cc: <two@example.com> # trailing comments are ignored
+ Cc: <three@example.com>, <not.four@example.com> one address per line
+ Cc: "Some # Body" <four@example.com> [ <also.a.comment> ]
EOF
clean_fake_sendmail &&
git send-email -1 --to=recipient@example.com \
--
2.11.1
^ permalink raw reply related
* Re: [PATCH v2] send-email: only allow one address per body tag
From: Matthieu Moy @ 2017-02-20 12:10 UTC (permalink / raw)
To: Johan Hovold; +Cc: Junio C Hamano, Jeff King, Kevin Daudt, Larry Finger, git
In-Reply-To: <20170220114406.19436-1-johan@kernel.org>
Johan Hovold <johan@kernel.org> writes:
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1563,7 +1563,7 @@ foreach my $t (@files) {
> # Now parse the message body
> while(<$fh>) {
> $message .= $_;
> - if (/^(Signed-off-by|Cc): (.*)$/i) {
> + if (/^(Signed-off-by|Cc): ([^>]*>?)/i) {
I think this is acceptable, but this doesn't work with trailers like
Cc: "Some > Body" <Some.Body@example.com>
A proper management of this kind of weird address should be doable by
reusing the regexp parsing "..." in parse_mailbox:
my $re_quote = qr/"(?:[^\"\\]|\\.)*"/;
So the final regex would look like
if (/^(Signed-off-by|Cc): (([^>]*|"(?:[^\"\\]|\\.)*")>?)/i) {
I don't think that should block the patch inclusion, but it may be worth
considering.
Anyway, thanks for the patch!
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH v4 14/15] files-backend: remove submodule_allowed from files_downcast()
From: Michael Haggerty @ 2017-02-20 12:11 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170218133303.3682-15-pclouds@gmail.com>
On 02/18/2017 02:33 PM, Nguyễn Thái Ngọc Duy wrote:
> Since submodule or not is irrelevant to files-backend after the last
> patch, remove the parameter from files_downcast() and kill
> files_assert_main_repository().
>
> PS. This implies that all ref operations are allowed for submodules. But
> we may need to look more closely to see if that's really true...
I think you are jumping the gun here.
Even after your changes, there is still a significant difference between
the main repository and submodules: we have access to the object
database for the main repository, but not for submodules.
So, for example, the following don't work for submodules:
* `parse_object()` is used to ensure that references are only pointed at
valid objects, and that branches are only pointed at commit objects.
* `peel_object()` is used to write the peeled version of references in
`packed-refs`, and to peel references while they are being iterated
over. Since this doesn't work, I think you can't write `packed-refs` in
a submodule.
These limitations, I think, imply that submodule references have to be
treated as read-only.
When I was working on a patch series similar to yours, I introduced a
boolean "main_repository" flag in `struct ref_store`, and use that
member to implement `files_assert_main_repository()`. That way
`files_ref_store::submodule` can still be removed, which is the more
important goal from a design standpoint.
Michael
^ permalink raw reply
* Re: [PATCH v4 14/15] files-backend: remove submodule_allowed from files_downcast()
From: Duy Nguyen @ 2017-02-20 12:21 UTC (permalink / raw)
To: Michael Haggerty
Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin,
Ramsay Jones, Stefan Beller, David Turner
In-Reply-To: <25fcb527-595a-7865-41e3-ee7c4c1ad668@alum.mit.edu>
On Mon, Feb 20, 2017 at 7:11 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 02/18/2017 02:33 PM, Nguyễn Thái Ngọc Duy wrote:
>> Since submodule or not is irrelevant to files-backend after the last
>> patch, remove the parameter from files_downcast() and kill
>> files_assert_main_repository().
>>
>> PS. This implies that all ref operations are allowed for submodules. But
>> we may need to look more closely to see if that's really true...
>
> I think you are jumping the gun here.
>
> Even after your changes, there is still a significant difference between
> the main repository and submodules: we have access to the object
> database for the main repository, but not for submodules.
>
> So, for example, the following don't work for submodules:
>
> * `parse_object()` is used to ensure that references are only pointed at
> valid objects, and that branches are only pointed at commit objects.
>
> * `peel_object()` is used to write the peeled version of references in
> `packed-refs`, and to peel references while they are being iterated
> over. Since this doesn't work, I think you can't write `packed-refs` in
> a submodule.
>
> These limitations, I think, imply that submodule references have to be
> treated as read-only.
Behind the scene submodule does add_submodule_odb(), which basically
makes the submodule's odb an alternate of in-core odb. So odb access
works. I was puzzled how submodules could by pass odb access at the
beginning only to find that out. technical/api-ref-iteration.txt also
mentions that you need to add_submodule_odb(), so I think it's
deliberate (albeit hacky) design.
> When I was working on a patch series similar to yours, I introduced a
> boolean "main_repository" flag in `struct ref_store`, and use that
> member to implement `files_assert_main_repository()`. That way
> `files_ref_store::submodule` can still be removed, which is the more
> important goal from a design standpoint.
I could keep the submodule check back (and replace the submodule
string in files_ref_store with just a flag). But I really think all
backend functions work with submodule. Perhaps add some tests to
exercise/verify that files-backend-on-submodule works?
--
Duy
^ permalink raw reply
* Re: [PATCH v2 04/16] files-backend: replace *git_path*() with files_path()
From: Duy Nguyen @ 2017-02-20 12:25 UTC (permalink / raw)
To: Michael Haggerty
Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin,
Ramsay Jones, Stefan Beller, David Turner
In-Reply-To: <330bfd35-ec00-92a2-a221-c7be9f0199e5@alum.mit.edu>
On Mon, Feb 20, 2017 at 6:23 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 02/16/2017 12:48 PM, Nguyễn Thái Ngọc Duy wrote:
>> This centralizes all path rewriting of files-backend.c in one place so
>> we have easier time removing the path rewriting later. There could be
>> some hidden indirect git_path() though, I didn't audit the code to the
>> bottom.
>
> Almost all of the calls to `files_path()` [1] take one of the following
> forms:
>
> * `files_path(refs, &sb, "packed-refs")`
> * `files_path(refs, &sb, "%s", refname)`
> * `files_path(refs, &sb, "logs/%s", refname)`
>
> (though sometimes `refname` is not the name of a reference but rather
> the name of a directory containing references, like "refs/heads").
>
> This suggests to me that it would be more natural to have three
> functions, `files_packed_refs_path()`, `files_loose_ref_path()`, and
> `files_reflog_path()`, with no `fmt` arguments. Aside from making the
> callers a bit simpler and the implementation of each of the three
> functions simpler (they wouldn't have to deal with variable argument
> lists), at the cost of needing three similar functions.
>
> But I think the split would also be advantageous from a design point of
> view. The relative path locations of loose reference files, reflog
> files, and the packed-refs file is kind of a coincidence, and it would
> be advantageous to encode that policy in three functions rather than
> continuing to spread knowledge of those assumptions around.
>
> It would also make it easier to switch to a new system of encoding
> reference names, for example so that reference names that differ only in
> case can be stored on a case-insensitive filesystem, or to store reflogs
> using a naming scheme that is not susceptible to D/F conflicts so that
> we can retain reflogs for deleted references.
I agree. I didn't see it clearly at the beginning (and made several
mistakes in earlier iterations) but as you have seen files_path() the
separation is pretty clear there. I was going to do it when
introducing the "linked worktree" backend. But since you pointed it
out, I'll update it in this series too.
> Michael
>
> [1] The only exception I see is one call `files_path(refs, &sb,
> "logs")`, which is a prelude to iterating over the reflog files. This is
> actually an example of the code giving us a hint that the design is
> wrong: if you iterate only over the directories under `git_path(logs)`,
> you miss the reflogs for worktree references.
Oh yes, I had to fix the reflog iterator [1] exactly for that :)
[1] https://public-inbox.org/git/20170217141908.18012-14-pclouds@gmail.com/T/#u
--
Duy
^ permalink raw reply
* Re: Git bisect does not find commit introducing the bug
From: Jakub Narębski @ 2017-02-20 12:27 UTC (permalink / raw)
To: Oleg Taranenko, Jacob Keller
Cc: Alex Hoffman, Johannes Sixt, Christian Couder, Stephan Beyer, git
In-Reply-To: <CABEd3j8sgDd8DXW8+2Q7pjANPTr-Ws1Xs1ap875mkxFOfnenYw@mail.gmail.com>
W dniu 20.02.2017 o 08:38, Oleg Taranenko pisze:
>>>> Then you must adjust your definition of "good": All commits that do not have
>>>> the feature, yet, are "good": since they do not have the feature in the
>>>> first place, they cannot have the breakage that you found in the feature.
>>>>
>>>> That is exactly the situation in your original example! But you constructed
>>>> the condition of goodness in such a simplistic way (depending on the
>>>> presence of a string), that it was impossible to distinguish between "does
>>>> not have the feature at all" and "has the feature, but it is broken".
>>>
>>> Johannes, thank you for correctly identifying the error in my logic.
>>> Indeed I was using the term 'bad' also for the commit without the
>>> feature. In order to find the commit introducing the bug in my example
>>> a new state is needed, which would make 'git bisect' a bit more
>>> complicated than the user 'most of the time' probably needs. Or do you
>>> think, it would make sense to ask the user for this state (if e.g 'git
>>> bisect' would be started with a new parameter)?
>
>> If a commit doesn't have the feature, then it is by definition, not
>> containing a broken feature, and you can simply use the "good" state.
>> There is no need for a different state. If you can't test the commit
>> because it's broken in some other way, you can use "git bisect skip"
>> but that isn't what you want in this case.
>
> Commits missing feature == 'good' commit is a very confusing one.
Nowadays you can change the names for 'old' and 'new' with
`git bisect terms`. HTH.
> Looks like in real life it happens much often, then git developers can
> imagine. For multi-branch/multi-feature workflow it's pretty easy not
> to recognize whether it is missing or not developed yet, especially on
> retrospective view where cherry-picking/squashing/merging is being
> used. My experience shows most annoying bugs are generating after a
> heavy merge (evil merge) with conflicts resolutions, where developer
> is not involved in the knowing what happens on counterpart changes.
> Then feature can be disappeared after it was worked & tested in its
> own branches.
Good to know about this problem.
> @Alex, I'm pretty interesting in fixing this weird bisect behaviour as
> well, as far as I struggled on it last summer and continue struggling
> so far :) If you want we can join to your efforts on fixing.
Anyway, I don't think it is feasible to weaken the assumption that there
is only one transition from 'old' ('good') to 'new' ('bad'); this is
what allows O(log(N)) operations. See bisection method of root finding,
that is finding zeros of a continuous function.
Best,
--
Jakub Narębski
^ 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