* [PATCH 1/3] diff.c: use diff_options directly
From: Stefan Beller @ 2016-09-07 23:36 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
In-Reply-To: <20160907233648.5162-1-sbeller@google.com>
The value of `ecbdata->opt` is accessible via the short variable `o`
already, so let's use that instead.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
diff.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/diff.c b/diff.c
index 534c12e..4a6501c 100644
--- a/diff.c
+++ b/diff.c
@@ -1217,7 +1217,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
const char *line_prefix = diff_line_prefix(o);
if (ecbdata->header) {
- fprintf(ecbdata->opt->file, "%s", ecbdata->header->buf);
+ fprintf(o->file, "%s", ecbdata->header->buf);
strbuf_reset(ecbdata->header);
ecbdata->header = NULL;
}
@@ -1229,9 +1229,9 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
name_a_tab = strchr(ecbdata->label_path[0], ' ') ? "\t" : "";
name_b_tab = strchr(ecbdata->label_path[1], ' ') ? "\t" : "";
- fprintf(ecbdata->opt->file, "%s%s--- %s%s%s\n",
+ fprintf(o->file, "%s%s--- %s%s%s\n",
line_prefix, meta, ecbdata->label_path[0], reset, name_a_tab);
- fprintf(ecbdata->opt->file, "%s%s+++ %s%s%s\n",
+ fprintf(o->file, "%s%s+++ %s%s%s\n",
line_prefix, meta, ecbdata->label_path[1], reset, name_b_tab);
ecbdata->label_path[0] = ecbdata->label_path[1] = NULL;
}
@@ -1249,15 +1249,15 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
find_lno(line, ecbdata);
emit_hunk_header(ecbdata, line, len);
if (line[len-1] != '\n')
- putc('\n', ecbdata->opt->file);
+ putc('\n', o->file);
return;
}
if (len < 1) {
- emit_line(ecbdata->opt, reset, reset, line, len);
+ emit_line(o, reset, reset, line, len);
if (ecbdata->diff_words
&& ecbdata->diff_words->type == DIFF_WORDS_PORCELAIN)
- fputs("~\n", ecbdata->opt->file);
+ fputs("~\n", o->file);
return;
}
@@ -1282,8 +1282,8 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
}
diff_words_flush(ecbdata);
if (ecbdata->diff_words->type == DIFF_WORDS_PORCELAIN) {
- emit_line(ecbdata->opt, context, reset, line, len);
- fputs("~\n", ecbdata->opt->file);
+ emit_line(o, context, reset, line, len);
+ fputs("~\n", o->file);
} else {
/*
* Skip the prefix character, if any. With
@@ -1294,7 +1294,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
line++;
len--;
}
- emit_line(ecbdata->opt, context, reset, line, len);
+ emit_line(o, context, reset, line, len);
}
return;
}
@@ -1316,8 +1316,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
default:
/* incomplete line at the end */
ecbdata->lno_in_preimage++;
- emit_line(ecbdata->opt,
- diff_get_color(ecbdata->color_diff, DIFF_CONTEXT),
+ emit_line(o, diff_get_color(ecbdata->color_diff, DIFF_CONTEXT),
reset, line, len);
break;
}
--
2.10.0.2.g0676c79.dirty
^ permalink raw reply related
* [PATCH 0/3] preparatory diff improvements for moved color detection
From: Stefan Beller @ 2016-09-07 23:36 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
Motivated by the feedback on the "[PATCHv4] diff.c: emit moved lines
with a different color"[1], I started refactoring the fn_out_consume function
in diff.c. This lead to a huge amount of tiny patches so far, but nothing
to present as an end result.
These patches are preparatory for this effort and I want them out such that
I do not need to worry about them later.
Thanks,
Stefan
[1] https://public-inbox.org/git/20160906070151.15163-1-stefanbeller@gmail.com/
Stefan Beller (3):
diff.c: use diff_options directly
diff: omit found pointer from emit_callback
diff: remove dead code
diff.c | 30 +++++++++---------------------
1 file changed, 9 insertions(+), 21 deletions(-)
--
2.10.0.2.g0676c79.dirty
^ permalink raw reply
* Re: [PATCH v3 0/8] Better heuristics make prettier diffs
From: Junio C Hamano @ 2016-09-07 23:25 UTC (permalink / raw)
To: Michael Haggerty
Cc: git, Ramsay Jones, René Scharfe, Stefan Beller, Jeff King,
Jakub Narębski, Jacob Keller
In-Reply-To: <cover.1473068229.git.mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> * Add test t4059 as part of this commit, not as part of its
> successor.
Which needs to be moved to somewhere else, as another topics that
has already been in 'next' uses t4059.
I'd move it temporarily to t4061 with a separate SQUASH??? at the
tip for now, as I am running out of time today.
^ permalink raw reply
* Re: [PATCH v2 00/20] object_id part 5
From: Junio C Hamano @ 2016-09-07 23:05 UTC (permalink / raw)
To: brian m. carlson
Cc: git, Paul Tan, Nguyễn Thái Ngọc Duy, Jeff King,
Johannes Schindelin, Jakub Narębski
In-Reply-To: <20160907222207.6fdorwi4q4p72gro@vauxhall.crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> On Wed, Sep 07, 2016 at 01:12:09PM -0700, Junio C Hamano wrote:
>> It is a bit unfortunate that this conflicts somewhat in builtin/cat-file.c
>> and heavily with builtin/apply.c with other topics in flight.
>>
>> Let me see how bad the conflict resolution is and report back later.
>
> If it ends up being problematic, feel free to drop the appropriate
> patches and I'll fix them up and send them in in the next set.
Let's see how well I do first ;-) There was another one in
wt-status.c that was touched by the "status --porcelain=v2" series,
but I haven't finished today's merges yet.
^ permalink raw reply
* Re: [PATCH v2 2/2] connect: advertized capability is not a ref
From: Junio C Hamano @ 2016-09-07 23:02 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Jonathan Tan, git, spearce, sbeller, peff
In-Reply-To: <20160907203836.GB25016@google.com>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Given that there aren't any servers that are going to produce this
> kind of bad input anyway, I prefer a die().
That would certainly put bigger pressure on the folks who write
buggy stuff in the future. If we know that nobody produces such
output, I'd prefer to forbid it, of course. It's just that is not
what this 2/2 implements ;-).
^ permalink raw reply
* Re: [RFC/PATCH v2 0/3] patch-id for merges
From: Josh Triplett @ 2016-09-07 22:51 UTC (permalink / raw)
To: Jeff King
Cc: git, Michael Haggerty, Kevin Willford, Xiaolong Ye,
Johannes Schindelin
In-Reply-To: <20160907220101.hwwutkiagfottbdd@sigill.intra.peff.net>
On Wed, Sep 07, 2016 at 06:01:01PM -0400, Jeff King wrote:
> Here's a re-roll of the series I posted at:
>
> http://public-inbox.org/git/20160907075346.z6wtmqnfc6bsunjb@sigill.intra.peff.net/
>
> Basically, it drops the time for "format-patch --cherry-pick" on a
> particular case from 3 minutes down to 3 seconds, by avoiding diffs
> on merge commits. Compared to v1, it fixes the totally-broken handling
> of commit_patch_id() pointed out by Johannes.
>
> We can drop the diffs on the merge commits because they're quite broken,
> as discussed in the commit message of patch 3 (they don't take into
> account any parent except the first). So what do we do when somebody
> asks for the patch-id of a merge commit?
>
> This is still marked RFC, because there are really two approaches here,
> and I'm not sure which one is better for "format-patch --base". I'd like
> to get input from Xiaolong Ye (who worked on --base), and Josh Triplett
> (who has proposed some patches in that area, and is presumably using
> them).
Thanks.
I'd love to see a more resilient patch-id mechanism, to make it easier
to match up patches between branches. I don't think it makes sense to
talk about the patch-id of a merge commit (though it might make sense
for a merge which makes additional changes not present in any of the
parents). Even if someone wants to match up merge commits with merge
commits, I don't think that should happen via patch-id; I think that
should happen in terms of "what patches does this merge introduce",
without constructing a merge-patch-id via a Merkle tree of commit
patch-ids.
So, I think this patch series makes sense (modulo the comments about the
commit message in patch 3). We already don't respect merge commits when
doing format-patch; this seems consistent with that. If we ever make it
possible for format-patch to handle merge commits, then we should also
allow it to have merge commits as prerequisites.
- Josh Triplett
^ permalink raw reply
* Re: [PATCH 3/3] patch-ids: use commit sha1 as patch-id for merge commits
From: Jeff King @ 2016-09-07 22:38 UTC (permalink / raw)
To: Jacob Keller
Cc: Git mailing list, Michael Haggerty, Kevin Willford, Xiaolong Ye,
Johannes Schindelin, Josh Triplett
In-Reply-To: <CA+P7+xr_GqU+v6RevEuqgNK5Kui=izRwD2oPvBh-4NzAp10uOQ@mail.gmail.com>
On Wed, Sep 07, 2016 at 03:28:10PM -0700, Jacob Keller wrote:
> On Wed, Sep 7, 2016 at 3:04 PM, Jeff King <peff@peff.net> wrote:
> > The patch-ids code which powers "log --cherry-pick" doesn't
> > look at whether each commit is a merge or not. It just feeds
> > the commit's first parent to the diff, and ignores any
> > additional parents.
> >
>
> The subject here is misleading since it says you will use sha1 of the
> merge commit ,but instead just ignore merge commits and indicate they
> have no patch id. I suspect this is because you switched
> implementations part way through developing this.
Oops, yes, that's exactly what happened. I'll wait for discussion to
settle on the approach and fix it up in the re-roll.
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] patch-ids: use commit sha1 as patch-id for merge commits
From: Jacob Keller @ 2016-09-07 22:28 UTC (permalink / raw)
To: Jeff King
Cc: Git mailing list, Michael Haggerty, Kevin Willford, Xiaolong Ye,
Johannes Schindelin, Josh Triplett
In-Reply-To: <20160907220439.ukbqnj5biaro2lxv@sigill.intra.peff.net>
On Wed, Sep 7, 2016 at 3:04 PM, Jeff King <peff@peff.net> wrote:
> The patch-ids code which powers "log --cherry-pick" doesn't
> look at whether each commit is a merge or not. It just feeds
> the commit's first parent to the diff, and ignores any
> additional parents.
>
The subject here is misleading since it says you will use sha1 of the
merge commit ,but instead just ignore merge commits and indicate they
have no patch id. I suspect this is because you switched
implementations part way through developing this.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH v2 00/20] object_id part 5
From: brian m. carlson @ 2016-09-07 22:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Paul Tan, Nguyễn Thái Ngọc Duy, Jeff King,
Johannes Schindelin, Jakub Narębski
In-Reply-To: <xmqq60q7lbae.fsf@gitster.mtv.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 593 bytes --]
On Wed, Sep 07, 2016 at 01:12:09PM -0700, Junio C Hamano wrote:
> It is a bit unfortunate that this conflicts somewhat in builtin/cat-file.c
> and heavily with builtin/apply.c with other topics in flight.
>
> Let me see how bad the conflict resolution is and report back later.
If it ends up being problematic, feel free to drop the appropriate
patches and I'll fix them up and send them in in the next set.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/3] patch-ids: turn off rename detection
From: Jacob Keller @ 2016-09-07 22:12 UTC (permalink / raw)
To: Jeff King
Cc: Git mailing list, Michael Haggerty, Kevin Willford, Xiaolong Ye,
Johannes Schindelin, Josh Triplett
In-Reply-To: <20160907220232.uotocvgdojfcl4o4@sigill.intra.peff.net>
On Wed, Sep 7, 2016 at 3:02 PM, Jeff King <peff@peff.net> wrote:
> The patch-id code may be running inside another porcelain
> like "git log" or "git format-patch", and therefore may have
> set diff_detect_rename_default, either via the diff-ui
> config, or by default since 5404c11 (diff: activate
> diff.renames by default, 2016-02-25). This is the case even
> if a command is run with `--no-renames`, as that is applied
> only to the diff-options used by the command itself.
>
> Rename detection doesn't help the patch-id results. It
> _may_ actually hurt, as minor differences in the files that
> would be overlooked by patch-id's canonicalization might
> result in different renames (though I'd doubt that it ever
> comes up in practice).
>
> But mostly it is just a waste of CPU to compute these
> renames.
Yes this seems reasonable.
>
> Note that this does have one user-visible impact: the
> prerequisite patches listed by "format-patch --base". There
> may be some confusion between different versions of git as
> older ones will enable renames, but newer ones will not.
> However, this was already a problem, as people with
> different settings for the "diff.renames" config would get
> different results. After this patch, everyone should get the
> same results, regardless of their config.
Makes sense.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> The patch is the same as v1, but the commit message is modified, as I
> realized that "--base" does expose this value publicly (but as I argue
> above, this is probably an improvement).
I agree this is an improvement.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 2/2] patch-ids: skip merge commits
From: Jeff King @ 2016-09-07 22:08 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Michael Haggerty, Kevin Willford
In-Reply-To: <20160907184653.ed5c4oklyywjj2gp@sigill.intra.peff.net>
On Wed, Sep 07, 2016 at 02:46:53PM -0400, Jeff King wrote:
> > With this change, commit_patch_id() will return 0 for merge commits
> > (indicating success) but it will not have touched the sha1! Which means it
> > may very well have all kinds of crap in the sha1 that may, or may not,
> > match another, real patch ID randomly.
>
> Eek, thanks. Somehow I got it into my head that diff_flush_patch_id()
> below was what added it to the list, but clearly that is not the case.
> Looking at it again, I can't imagine how that is the case.
Ah, I see. I initially was looking at an older git (v2.6.x), in which
commit_patch_id() is a static function inside patch-ids.c, and we do not
do any lazy-load trickery. But note that the patch is still wrong even
there; it should return "-1" from commit_patch_id() to instruct the
caller not to add it to the hash.
Anyway...
> > I would suggest to simply copy the merge commit's SHA-1. It is no patch
> > ID, of course, but collisions are as unlikely as commit name collisions,
> > and it would make the "patch ID" of a merge commit deterministic again.
>
> I agree that would work, though it does mean carrying extra useless
> entries in the patch_id hash. I'll see how bad it would be to simply
> omit them entirely, but this seems like a good fallback plan.
It's not too hard to do so, but it raises a question of what
"format-patch --base" would want. I've just sent another RFC cc-ing
folks interested in that area.
Thanks again for the review.
-Peff
^ permalink raw reply
* [PATCH 3/3] patch-ids: use commit sha1 as patch-id for merge commits
From: Jeff King @ 2016-09-07 22:04 UTC (permalink / raw)
To: git
Cc: Michael Haggerty, Kevin Willford, Xiaolong Ye,
Johannes Schindelin, Josh Triplett
In-Reply-To: <20160907220101.hwwutkiagfottbdd@sigill.intra.peff.net>
The patch-ids code which powers "log --cherry-pick" doesn't
look at whether each commit is a merge or not. It just feeds
the commit's first parent to the diff, and ignores any
additional parents.
In theory, this might be useful if you wanted to find
equivalence between, say, a merge commit and a squash-merge
that does the same thing. But it also promotes a false
equivalence between distinct merges; for example, every
"merge -s ours" would look like an empty commit (which is
true in a sense, but presumably there was a value in merging
in the discarded history). Since patch-ids are meant for
throwing away duplicates, we should err on the side of _not_
matching such merges.
Moreover, we may spend a lot of extra time computing these
merge diffs. In the case that inspired this patch, a "git
format-patch --cherry-pick" dropped from over 3 minutes to
less than 4 seconds.
This seems pretty drastic, but is easily explained. The
command was invoked by a "git rebase" of an older topic
branch; there had been tens of thousands of commits on the
upstream branch in the meantime. In addition, this project
used a topic-branch workflow with occasional "back-merges"
from "master" to each topic (to resolve conflicts on the
topics rather than in the merge commits). So there were not
only extra merges, but the diffs for these back-merges were
generally quite large (because they represented _everything_
that had been merged to master since the topic branched).
This patch defines the patch-id of a merge commit as
essentially "null"; it has no patch-id. As a result,
merges cannot match patch-ids via "--cherry-pick", and
"format-patch --base" will not list merges in its list of
prerequisite patch ids.
We can signal the null patch-id by returning "-1" from
commit_patch_id(), as it no longer returns any meaningful
errors (and as a result, its callers are updated to handle
"-1" differently).
Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/log.c | 2 +-
patch-ids.c | 14 +++++++-------
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 92dc34d..e660034 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1344,7 +1344,7 @@ static void prepare_bases(struct base_tree_info *bases,
if (commit->util)
continue;
if (commit_patch_id(commit, &diffopt, sha1, 0))
- die(_("cannot get patch id"));
+ continue;
ALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, bases->alloc_patch_id);
patch_id = bases->patch_id + bases->nr_patch_id;
hashcpy(patch_id->hash, sha1);
diff --git a/patch-ids.c b/patch-ids.c
index 0e95220..197c70b 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -7,10 +7,12 @@
int commit_patch_id(struct commit *commit, struct diff_options *options,
unsigned char *sha1, int diff_header_only)
{
- if (commit->parents)
+ if (commit->parents) {
+ if (commit->parents->next)
+ return -1;
diff_tree_sha1(commit->parents->item->object.oid.hash,
commit->object.oid.hash, "", options);
- else
+ } else
diff_root_tree_sha1(commit->object.oid.hash, "", options);
diffcore_std(options);
diff_flush_patch_id(options, sha1, diff_header_only);
@@ -19,7 +21,7 @@ int commit_patch_id(struct commit *commit, struct diff_options *options,
/*
* When we cannot load the full patch-id for both commits for whatever
- * reason, the function returns -1 (i.e. return error(...)). Despite
+ * reason, the function returns -1. Despite
* the "cmp" in the name of this function, the caller only cares about
* the return value being zero (a and b are equivalent) or non-zero (a
* and b are different), and returning non-zero would keep both in the
@@ -33,12 +35,10 @@ static int patch_id_cmp(struct patch_id *a,
{
if (is_null_sha1(a->patch_id) &&
commit_patch_id(a->commit, opt, a->patch_id, 0))
- return error("Could not get patch ID for %s",
- oid_to_hex(&a->commit->object.oid));
+ return -1;
if (is_null_sha1(b->patch_id) &&
commit_patch_id(b->commit, opt, b->patch_id, 0))
- return error("Could not get patch ID for %s",
- oid_to_hex(&b->commit->object.oid));
+ return -1;
return hashcmp(a->patch_id, b->patch_id);
}
--
2.10.0.rc2.154.gb4a4b8b
^ permalink raw reply related
* [PATCH 2/3] diff_flush_patch_id: stop returning error result
From: Jeff King @ 2016-09-07 22:04 UTC (permalink / raw)
To: git
Cc: Michael Haggerty, Kevin Willford, Xiaolong Ye,
Johannes Schindelin, Josh Triplett
In-Reply-To: <20160907220101.hwwutkiagfottbdd@sigill.intra.peff.net>
All of our errors come from diff_get_patch_id(), which has
exactly three error conditions. The first is an internal
assertion, which should be a die("BUG") in the first place.
The other two are caused by an inability to two diff blobs,
which is an indication of a serious problem (probably
repository corruption). All the rest of the diff subsystem
dies immediately on these conditions. By passing up the
error, in theory we can keep going even if patch-id is
unable to function. But in practice this means we may
generate subtly wrong results (e.g., by failing to correlate
two commits). Let's just die(), as we're better off making
it clear to the user that their repository is not
functional.
As a result, we can simplify the calling code.
Signed-off-by: Jeff King <peff@peff.net>
---
This is a prerequisite for patch 3, since it means that
commit_patch_id() stops returning "real" errors. But obviously if this
is distasteful (and it does feel a little weird to convert error() to
die(), even though the rest of the diff code-base behaves this way), we
can teach commit_patch_id() to distinguish between "this has no
patch-id" and "a real error occured" in its return value.
diff.c | 18 ++++++++----------
diff.h | 2 +-
patch-ids.c | 3 ++-
3 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/diff.c b/diff.c
index 534c12e..d0594f6 100644
--- a/diff.c
+++ b/diff.c
@@ -4462,7 +4462,7 @@ static void patch_id_consume(void *priv, char *line, unsigned long len)
}
/* returns 0 upon success, and writes result into sha1 */
-static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1, int diff_header_only)
+static void diff_get_patch_id(struct diff_options *options, unsigned char *sha1, int diff_header_only)
{
struct diff_queue_struct *q = &diff_queued_diff;
int i;
@@ -4484,7 +4484,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1,
memset(&xpp, 0, sizeof(xpp));
memset(&xecfg, 0, sizeof(xecfg));
if (p->status == 0)
- return error("internal diff status error");
+ die("BUG: diff status unset while computing patch_id");
if (p->status == DIFF_STATUS_UNKNOWN)
continue;
if (diff_unmodified_pair(p))
@@ -4536,7 +4536,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1,
if (fill_mmfile(&mf1, p->one) < 0 ||
fill_mmfile(&mf2, p->two) < 0)
- return error("unable to read files to diff");
+ die("unable to read files to diff");
if (diff_filespec_is_binary(p->one) ||
diff_filespec_is_binary(p->two)) {
@@ -4552,27 +4552,25 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1,
xecfg.flags = 0;
if (xdi_diff_outf(&mf1, &mf2, patch_id_consume, &data,
&xpp, &xecfg))
- return error("unable to generate patch-id diff for %s",
- p->one->path);
+ die("unable to generate patch-id diff for %s",
+ p->one->path);
}
git_SHA1_Final(sha1, &ctx);
- return 0;
}
-int diff_flush_patch_id(struct diff_options *options, unsigned char *sha1, int diff_header_only)
+void diff_flush_patch_id(struct diff_options *options, unsigned char *sha1, int diff_header_only)
{
struct diff_queue_struct *q = &diff_queued_diff;
int i;
- int result = diff_get_patch_id(options, sha1, diff_header_only);
+
+ diff_get_patch_id(options, sha1, diff_header_only);
for (i = 0; i < q->nr; i++)
diff_free_filepair(q->queue[i]);
free(q->queue);
DIFF_QUEUE_CLEAR(q);
-
- return result;
}
static int is_summary_empty(const struct diff_queue_struct *q)
diff --git a/diff.h b/diff.h
index 7883729..f4dcfe1 100644
--- a/diff.h
+++ b/diff.h
@@ -342,7 +342,7 @@ extern int run_diff_files(struct rev_info *revs, unsigned int option);
extern int run_diff_index(struct rev_info *revs, int cached);
extern int do_diff_cache(const unsigned char *, struct diff_options *);
-extern int diff_flush_patch_id(struct diff_options *, unsigned char *, int);
+extern void diff_flush_patch_id(struct diff_options *, unsigned char *, int);
extern int diff_result_code(struct diff_options *, int);
diff --git a/patch-ids.c b/patch-ids.c
index 77e4663..0e95220 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -13,7 +13,8 @@ int commit_patch_id(struct commit *commit, struct diff_options *options,
else
diff_root_tree_sha1(commit->object.oid.hash, "", options);
diffcore_std(options);
- return diff_flush_patch_id(options, sha1, diff_header_only);
+ diff_flush_patch_id(options, sha1, diff_header_only);
+ return 0;
}
/*
--
2.10.0.rc2.154.gb4a4b8b
^ permalink raw reply related
* [PATCH 1/3] patch-ids: turn off rename detection
From: Jeff King @ 2016-09-07 22:02 UTC (permalink / raw)
To: git
Cc: Michael Haggerty, Kevin Willford, Xiaolong Ye,
Johannes Schindelin, Josh Triplett
In-Reply-To: <20160907220101.hwwutkiagfottbdd@sigill.intra.peff.net>
The patch-id code may be running inside another porcelain
like "git log" or "git format-patch", and therefore may have
set diff_detect_rename_default, either via the diff-ui
config, or by default since 5404c11 (diff: activate
diff.renames by default, 2016-02-25). This is the case even
if a command is run with `--no-renames`, as that is applied
only to the diff-options used by the command itself.
Rename detection doesn't help the patch-id results. It
_may_ actually hurt, as minor differences in the files that
would be overlooked by patch-id's canonicalization might
result in different renames (though I'd doubt that it ever
comes up in practice).
But mostly it is just a waste of CPU to compute these
renames.
Note that this does have one user-visible impact: the
prerequisite patches listed by "format-patch --base". There
may be some confusion between different versions of git as
older ones will enable renames, but newer ones will not.
However, this was already a problem, as people with
different settings for the "diff.renames" config would get
different results. After this patch, everyone should get the
same results, regardless of their config.
Signed-off-by: Jeff King <peff@peff.net>
---
The patch is the same as v1, but the commit message is modified, as I
realized that "--base" does expose this value publicly (but as I argue
above, this is probably an improvement).
patch-ids.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/patch-ids.c b/patch-ids.c
index 082412a..77e4663 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -45,6 +45,7 @@ int init_patch_ids(struct patch_ids *ids)
{
memset(ids, 0, sizeof(*ids));
diff_setup(&ids->diffopts);
+ ids->diffopts.detect_rename = 0;
DIFF_OPT_SET(&ids->diffopts, RECURSIVE);
diff_setup_done(&ids->diffopts);
hashmap_init(&ids->patches, (hashmap_cmp_fn)patch_id_cmp, 256);
--
2.10.0.rc2.154.gb4a4b8b
^ permalink raw reply related
* [RFC/PATCH v2 0/3] patch-id for merges
From: Jeff King @ 2016-09-07 22:01 UTC (permalink / raw)
To: git
Cc: Michael Haggerty, Kevin Willford, Xiaolong Ye,
Johannes Schindelin, Josh Triplett
In-Reply-To: <20160907075346.z6wtmqnfc6bsunjb@sigill.intra.peff.net>
Here's a re-roll of the series I posted at:
http://public-inbox.org/git/20160907075346.z6wtmqnfc6bsunjb@sigill.intra.peff.net/
Basically, it drops the time for "format-patch --cherry-pick" on a
particular case from 3 minutes down to 3 seconds, by avoiding diffs
on merge commits. Compared to v1, it fixes the totally-broken handling
of commit_patch_id() pointed out by Johannes.
We can drop the diffs on the merge commits because they're quite broken,
as discussed in the commit message of patch 3 (they don't take into
account any parent except the first). So what do we do when somebody
asks for the patch-id of a merge commit?
This is still marked RFC, because there are really two approaches here,
and I'm not sure which one is better for "format-patch --base". I'd like
to get input from Xiaolong Ye (who worked on --base), and Josh Triplett
(who has proposed some patches in that area, and is presumably using
them).
Option one is that merges are defined as having no patch-id at all. They
are skipped for "--cherry-pick" comparison, and "format-patch --base"
will not mention them at all as prerequisites. That's what I've
implemented here.
Option two is to use the commit sha1 as the patch-id for a merge, making
it (essentially) unique. That gives us a defined value, but it's one
that "--cherry-pick" will not match between two segments of history. I
don't know if having _some_ defined value is useful for "format-patch
--base" or not.
And obviously there's an option 3: define some more complicated patch-id
for merges that takes into account all of the parents. I didn't think
too much on that because I don't really see value in it over using the
commit sha1, and it would be computationally expensive.
[1/3]: patch-ids: turn off rename detection
[2/3]: diff_flush_patch_id: stop returning error result
[3/3]: patch-ids: use commit sha1 as patch-id for merge commits
-Peff
^ permalink raw reply
* Re: [PATCH v3 0/8] Better heuristics make prettier diffs
From: Jacob Keller @ 2016-09-07 21:21 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael Haggerty, Git mailing list, Ramsay Jones,
René Scharfe, Stefan Beller, Jeff King, Jakub Narębski
In-Reply-To: <xmqqy433mvg6.fsf@gitster.mtv.corp.google.com>
On Wed, Sep 7, 2016 at 11:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> * In "blame: honor the diff heuristic options and config":
>>
>> * In v2, I suggested making `blame` honor all diff-related options.
>> Junio explained why this was a bad idea. So this version only
>> makes `blame` honor `--indent-heuristic` and
>> `--compaction-heuristic`.
>
> This makes a lot more sense to me. I am not sure if it is worth
> adding 7/8 to expose these experimental knobs to the end users, but
> the amount of the code needed is very small, so let's take it.
>
> I am hoping that we can lose both of the experimental knobs and use
> the indent heuristic unconditionally in the future, though ;-)
>
> Thanks.
>
Agreed, I think we should drop the knobs and use the feature
unconditionally as well.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCHv3] diff.c: emit moved lines with a different color
From: Jacob Keller @ 2016-09-07 21:21 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, Stefan Beller, Git mailing list,
Jakub Narębski
In-Reply-To: <CAGZ79kbSG-9xdmG3LWEsVoJnhEr5pcL8D-95E99a+=JaXGOXCA@mail.gmail.com>
On Wed, Sep 7, 2016 at 11:02 AM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, Sep 7, 2016 at 10:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> as we do not want to see dashes ('moved-old'), I think I'l go with
>>> "movedfrom" and "movedto".
>>
>> OK. They would be color.diff.movedFrom and color.diff.movedTo in
>> the doc, and "movedfrom" and "movedto" in the code (as the caller
>> already downcased them for you to strcmp()).
>>
>>> When going through the second pass and actually emitting colored lines
>>> we only find matches in the hash map if the current line AND the previous line
>>> match as we lookup by hash code, i.e. if we have a moved line, but the
>>> previous line
>>> changed we do not find it in the hashmap and we don't color it, such
>>> that the reviewer
>>> can spot a permutation.
>>
>> Hmph. Does this have impact on a line that was at the beginning or
>> the end of a file that got moved to the beginning or the end of a
>> file (four permutations, of 9 if you throw in "middle"), because
>> some cases it does not have a "previous" line?
>
> I spotted that problem as well. We need to adapt the algorithm a bit more:
>
> If the previous line is of the same kind (i.e. starting with + or -),
> then we have to take it into account,
> otherwise (i.e. previous line is different, such as header, hunk header,
> or different sign)
> then ignore previous line, as the previous line is already
> having a different color.
>
> That works for the very first line of a file as well.
That should work to resolve some issues, and ensure the first line of
a hunk works correctly.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH v2 2/2] connect: advertized capability is not a ref
From: Jonathan Nieder @ 2016-09-07 20:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Tan, git, spearce, sbeller, peff
In-Reply-To: <xmqqoa3zocud.fsf@gitster.mtv.corp.google.com>
Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>> diff --git a/connect.c b/connect.c
>> index 722dc3f..0c2221e 100644
>> --- a/connect.c
>> +++ b/connect.c
>> @@ -165,6 +165,9 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>> continue;
>> }
>>
>> + if (!strcmp(name, "capabilities^{}"))
>> + continue;
>
> While it is true that ignoring this line anywhere in the ref
> advertisement is safe, it feels a bit strange to do so, when we know
> that it can appear _only_ when there is no other ref advertised. I
> guess you can argue that it is good to be lenient to accept what
> others produce, but on the other hand, it can also be argued that
> having this among real ref advertisement would be a protocol
> violation that we may want to diagnose and prod the other side to
> fix their software (but still not fail).
By "it can also be argued", do you mean that you would prefer that
behavior?
It sounds like the worst of both worlds to me --- git would allow the
buggy server behavior, leading people not to fix their servers, but it
would print an ugly error message, so end-users would associate git
with confusing messages.
Given that there aren't any servers that are going to produce this
kind of bad input anyway, I prefer a die().
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH v2 00/20] object_id part 5
From: Junio C Hamano @ 2016-09-07 20:12 UTC (permalink / raw)
To: brian m. carlson
Cc: git, Paul Tan, Nguyễn Thái Ngọc Duy, Jeff King,
Johannes Schindelin, Jakub Narębski
In-Reply-To: <20160905200811.697889-1-sandals@crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> This is the fifth in a series of series to convert from unsigned char [20] to
> struct object_id.
>
> This series converts many of the files in the builtin directory to use struct
> object_id. This gets us almost to the point where we can convert get_tree_entry
> to use struct object_id, but not quite. That function is used indirectly by
> get_sha1, meaning that get_oid would have to completely replace it in order for
> get_tree_entry to be converted.
>
> However, this series tackles one of two major sources of object ID values: the
> command line (the other, of course, being the refs code). Converting several of
> the builtin commands to use struct object_id as much as possible makes it easier
> to convert other functions down the line.
It is a bit unfortunate that this conflicts somewhat in builtin/cat-file.c
and heavily with builtin/apply.c with other topics in flight.
Let me see how bad the conflict resolution is and report back later.
Thanks.
^ permalink raw reply
* Re: segfault in http.c when https URL is mistyped
From: Jeff King @ 2016-09-07 20:11 UTC (permalink / raw)
To: Lars Wendler; +Cc: Junio C Hamano, Knut Franke, git, Robin H. Johnson, lekto
In-Reply-To: <20160907200642.5o3otxxt7ybw6x4c@sigill.intra.peff.net>
On Wed, Sep 07, 2016 at 04:06:42PM -0400, Jeff King wrote:
> +test_expect_success 'remote-http complains cleanly about malformed urls' '
> + # do not actually issue "list" or other commands, as we do not
> + # want to rely on what curl would actually do with such a broken
> + # URL. This is just about making sure we do not segfault during
> + # initialization.
> + test_must_fail git remote-http http::/example.com/repo.git
> +'
Actually, I guess remote-http actually sees just "/example.com/repo.git"
for the case we're discussing. It segfaults with either input without
this patch, and works with it, though.
(I didn't test "git clone" directly because it hides the segfault made
by the remote helper, so we cannot tell the difference between a
segfault and "this URL is broken").
-Peff
^ permalink raw reply
* Re: segfault in http.c when https URL is mistyped
From: Jeff King @ 2016-09-07 20:06 UTC (permalink / raw)
To: Lars Wendler; +Cc: Junio C Hamano, Knut Franke, git, Robin H. Johnson, lekto
In-Reply-To: <20160907154404.107fe6e6@abudhabi.paradoxon.rec>
On Wed, Sep 07, 2016 at 03:44:04PM +0200, Lars Wendler wrote:
> we at Gentoo got a bug report [1] about git-remote-https segfaulting
> when the URL has been mistyped.
> This seems to only be triggered when git was compiled with curl
> support:
>
> git clone https::/some.example-site.net/test.git
Thanks, this was easy to reproduce. It's a regression in v2.8.0. The fix
is below.
-- >8 --
Subject: [PATCH] remote-curl: handle URLs without protocol
Generally remote-curl would never see a URL that did not
have "proto:" at the beginning, as that is what tells git to
run the "git-remote-proto" helper (and git-remote-http, etc,
are aliases for git-remote-curl).
However, the special syntax "proto::something" will run
git-remote-proto with only "something" as the URL. So a
malformed URL like:
http::/example.com/repo.git
will feed the URL "/example.com/repo.git" to
git-remote-http. The resulting URL has no protocol, but the
code added by 372370f (http: use credential API to handle
proxy authentication, 2016-01-26) does not handle this case
and segfaults.
For the purposes of this code, we don't really care what the
exact protocol; only whether or not it is https. So let's
just assume that a missing protocol is not, and curl will
handle the real error (which is that the URL is nonsense).
Signed-off-by: Jeff King <peff@peff.net>
---
I looked around for other similar over-assumptions about the URL parsing
but didn't see any.
http.c | 2 +-
t/t5550-http-fetch-dumb.sh | 8 ++++++++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/http.c b/http.c
index cd40b01..edce47c 100644
--- a/http.c
+++ b/http.c
@@ -723,7 +723,7 @@ static CURL *get_curl_handle(void)
* precedence here, as in CURL.
*/
if (!curl_http_proxy) {
- if (!strcmp(http_auth.protocol, "https")) {
+ if (http_auth.protocol && !strcmp(http_auth.protocol, "https")) {
var_override(&curl_http_proxy, getenv("HTTPS_PROXY"));
var_override(&curl_http_proxy, getenv("https_proxy"));
} else {
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 3484b6f..01bb633 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -299,5 +299,13 @@ test_expect_success 'git client does not send an empty Accept-Language' '
! grep "^Accept-Language:" stderr
'
+test_expect_success 'remote-http complains cleanly about malformed urls' '
+ # do not actually issue "list" or other commands, as we do not
+ # want to rely on what curl would actually do with such a broken
+ # URL. This is just about making sure we do not segfault during
+ # initialization.
+ test_must_fail git remote-http http::/example.com/repo.git
+'
+
stop_httpd
test_done
--
2.10.0.rc2.154.gb4a4b8b
^ permalink raw reply related
* Re: [PATCH 3/4] t5550-http-fetch-dumb.sh: use the GIT_TRACE_CURL environment var
From: Junio C Hamano @ 2016-09-07 19:58 UTC (permalink / raw)
To: Elia Pinto; +Cc: Eric Sunshine, Git List
In-Reply-To: <xmqqpoofmuru.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Elia Pinto <gitter.spiros@gmail.com> writes:
>
>>>> + cp expect expect.$$ &&
>>>> + cp actual actual.$$ &&
>>>> + cp output output.$$ &&
>>>
>>> What are these three cp's about? They don't seem to be related to the
>>> stated changes. Are they leftover debugging gunk?
>> Yes, i am very sorry. My bad. I will repost. Thanks
>
> Nevertheless, thanks for these clean-ups. Will discard this round
> waiting for a reroll.
Nevermind. I see you rerolled only this one labeled as "v1 3/4";
picked up the rest from this series and combined with that reroll.
Thanks.
^ permalink raw reply
* Re: [PATCH] t6026-merge-attr: wait for process to release trash directory
From: Junio C Hamano @ 2016-09-07 19:59 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, Johannes Schindelin, Git Mailing List
In-Reply-To: <20160907190004.dw3p6fxkdaubwuvu@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I agree that the sleep could be made longer, to make the test less racy.
> However, the racy failure mode is that it might pass while testing
> nothing (i.e., the sleep ends anyway before the hook returns), so I
> don't think it's a high priority.
I do not think it is necessary, either. It was just me wondering
aloud, nothing more.
^ permalink raw reply
* Re: [PATCH v14 00/41] libify apply and use lib in am, part 2
From: Junio C Hamano @ 2016-09-07 19:51 UTC (permalink / raw)
To: Christian Couder
Cc: git, Jeff King, Ævar Arnfjörð Bjarmason,
Karsten Blees, Nguyen Thai Ngoc Duy, Stefan Beller, Eric Sunshine,
Ramsay Jones, Johannes Sixt, René Scharfe, Stefan Naewe,
Christian Couder
In-Reply-To: <20160904201833.21676-1-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> In patch 29/41 I added some comments in apply.h above the definition
> of APPLY_OPT_INACCURATE_EOF and APPLY_OPT_RECOUNT, as suggested by
> Stefan. This is the only change compared to v13.
OK.
> - Patches 33/41 to 37/41 were in v10, v12 and v13.
>
> They implement a way to make the libified apply code silent by
> changing the bool `apply_verbosely` into a tristate enum called
> `apply_verbosity`, that can be one of `verbosity_verbose`,
> `verbosity_normal` or `verbosity_silent`.
This is a reasonable approach.
> The only changes since v13 are in 37/41. The name of the first
> argument to mute_routine() is changed from "bla" to "msg" as suggested
> by Ramsey, and the commit message is improved as suggested by Stefan.
;-)
> - Patch 40/41 was in v12 and v13, and hasn't changed.
>
> It adds a "const char *index_file" into "struct apply_state", to make
> it possible to use a special index file instead of the default one.
Looks sensible.
^ permalink raw reply
* Re: [PATCH 5/5] pack-objects: walk tag chains for --include-tag
From: Jeff King @ 2016-09-07 19:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqzinjlf47.fsf@gitster.mtv.corp.google.com>
On Wed, Sep 07, 2016 at 11:49:28AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > As explained further in the commit message, "fetch" is robust to this,
> > because it does a real connectivity check and follow-on fetch before
> > writing anything it thinks it got via include-tag. So perhaps one could
> > argue that pack-objects is correct; include-tag is best-effort, and it
> > is the client's job to make sure it has everything it needs. And that
> > would mean the bug is in git-clone, which should be doing the
> > connectivity check and follow-on fetch.
>
> I think that is probably a more technically correct interpretation
> of the history.
>
> I think upgrading "best-effort" to "guarantee" like you did is a
> right approach nevertheless. I think the "best-effort" we initially
> did was merely us being lazy.
Yeah, after sleeping on it, the conclusion I came to was that it does
not _hurt_ to have include-tag be a bit more careful.
I also wondered about the corner case I noted in the commit message. If
you have a tag chain of A->B->C, and you already have "C" (a commit),
but are fetching "B" (a tag), then include-tag does not notice "A".
That's OK for git-fetch. It will collect "A" during its backfill phase
(not because of "B" at all, but because it knows that "A" eventually
peels to "C", which it already has). "git-clone" does not have a
backfill, of course. But neither can it "already have" a commit. So
either we get "C" as part of the clone (in which case include-tag will
include "A"), or it does not (in which case we cannot be getting "B"
either, because "C" is reachable from it).
And of course that's only when single-branch is in use. Normally
git-clone just grabs all the tags blindly. :)
So I think everything Just Works after my patch, though we do still rely
on fetch backfill to pick up some obscure cases.
-Peff
^ 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