* 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
* Re: [PATCH v2 00/38] Virtualization of the refs API
From: Junio C Hamano @ 2016-09-07 19:20 UTC (permalink / raw)
To: Michael Haggerty
Cc: David Turner, Ramsay Jones, Eric Sunshine, Jeff King,
Nguyễn Thái Ngọc Duy, git
In-Reply-To: <cover.1473003902.git.mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> This is v2 of the patch series to virtualize the references API
> (though earlier patch series similar in spirit were submitted by
> Ronnie Sahlberg and David Turner). Thanks to Junio, Eric, and Ramsay
> for their comments about v1 [1].
>
> Nobody pointed out any fundamental problems with v1, but this version
> includes the following improvements:
Curiously, many of these improvements were already in 'pu'.
> * In "rename_ref_available(): add docstring":
>
> * Improve docstring as suggested by Junio.
I might have squashed that in myself; it is a long time ago so I
don't quite remember.
> * In "refs: create a base class "ref_store" for files_ref_store":
>
> * Let main_ref_store and submodule_ref_stores be initialized
> implicitly rather than initializing them explicitly to NULL.
>
> * Add docstrings for those two variables.
>
> * Eliminate a temporary variable in `files_downcast()`.
This was already in what has been in 'pu'.
> * In "resolve_gitlink_ref(): avoid memory allocation in many cases":
>
> * Instead of keeping track of `orig_len`, after stripping off any
> trailing slashes check whether `path[len - 1]` is NUL.
This was already in what has been in 'pu'.
> * In "refs: add methods for reflog":
>
> * Don't export `files_reflog_iterator_begin()` (suggested by
> Ramsay).
This I can see was missing in what has been in 'pu'.
^ permalink raw reply
* Re: [PATCH] t6026-merge-attr: wait for process to release trash directory
From: Jeff King @ 2016-09-07 19:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Johannes Schindelin, Git Mailing List
In-Reply-To: <xmqq4m5rmu4i.fsf@gitster.mtv.corp.google.com>
On Wed, Sep 07, 2016 at 11:39:57AM -0700, Junio C Hamano wrote:
> > Can we do some signaling with fifos to tell the hook when it is safe to
> > exit? Then we would just need to `wait` for its parent process.
>
> Is fifo safe on Windows, though?
No clue. We seem to use mkfifo unconditionally in lib-daemon, but
perhaps people do not run that test on Windows. Other invocations seem
to be protected by the PIPE prerequisite. But...
> With v2 that explicitly kills, I guess we can make the sleep longer
> without slowing down in the optimistic case?
Yeah, I think the v2 one is non-racy (I thought at first we might race
with the "echo", but it should be synchronous; the hook will not exit
until we have written the pid file, and git will not exit until the hook
is done running).
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.
-Peff
^ permalink raw reply
* Re: [PATCH v2] rebase -i: improve advice on bad instruction lines
From: Junio C Hamano @ 2016-09-07 18:58 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: Ralf Thielow, git
In-Reply-To: <1473240491.20551.1.camel@kaarsemaker.net>
Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> There are quite a few patch series in flight these days around
> interactive rebase. Have you checked for conflicts with those?
IIRC, Ralf's fix would need to be ported to C version, and when it
happens, it won't just be a mere conflict but will disappear from
the scripted version ;-)
But the C version is not anywhere near 'pu' yet and being rerolled,
so I do not mind queuing it on 'pu' now merely as a reminder.
^ permalink raw reply
* Re: [PATCHv4] diff.c: emit moved lines with a different color
From: Junio C Hamano @ 2016-09-07 18:52 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Stefan Beller, git, jnareb, jacob.keller, Stefan Beller
In-Reply-To: <87a1ddbf-2499-d5b4-55c4-aeed2b72acce@ramsayjones.plus.com>
Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>> +static int diff_line_moved_entry_cmp(const struct diff_line_moved_entry *a,
>> + const struct diff_line_moved_entry *b,
>> + const void *unused)
>> +{
>> + return strcmp(a->line, b->line) &&
>> + a->hash_prev_line == b->hash_prev_line;
>
> I doubt it would make much difference, but my knee-jerk reaction to
> this was to suggest swapping the order of the expression, thus:
>
> return a->hash_prev_line == b->hash_prev_line &&
> strcmp(a->line, b->line);
>
> ... but perhaps it doesn't read quite so well, and probably wouldn't affect
> performance much (except in strange edge cases), so it may not be worth it.
It would make very much sense to do so, as the final version will be
a lot more involved than a mere strcmp() to make "git diff -w" to
also work as expected with this new feature.
^ permalink raw reply
* Re: [PATCH 5/5] pack-objects: walk tag chains for --include-tag
From: Junio C Hamano @ 2016-09-07 18:49 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20160905215939.hriu6ev3m332qhp6@sigill.intra.peff.net>
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.
^ permalink raw reply
* Re: [RFC/PATCH 0/2] more patch-id speedups
From: Jeff King @ 2016-09-07 18:49 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Michael Haggerty, Kevin Willford
In-Reply-To: <alpine.DEB.2.20.1609071453240.129229@virtualbox>
On Wed, Sep 07, 2016 at 03:06:36PM +0200, Johannes Schindelin wrote:
> > This is marked as "RFC" because I don't feel entirely confident that I'm
> > not missing some clever need for these options. But in both cases my gut
> > feeling is that they are simply unintended effects that nobody ever
> > noticed, because it would be very rare that they would affect the
> > output. And that if they _did_ affect the output, they would probably be
> > doing the wrong thing.
>
> Given that the patch ID is *wrong* for merge commits (it only looks at the
> first parent, so each "-s ours" merge will have the same patch ID!), I
> would say that we can get away with re-defining the patch ID of merge
> commits.
>
> The only case where it might change things that I can think of would be a
> `git rebase --preserve-merges`: it would probably have worked *by chance*
> before (or not, in case of "-s ours" merges), and now it would try to pick
> the merge commits even if rebased versions were already merged upstream.
>
> If I read the --preserve-merges code correctly, that would result in the
> merge commit's parents to be 'rewritten' to HEAD. And as both parents
> would be rewritten to HEAD, they would be condensed into a single new
> parent, resulting in a cherry-pick that fails (because it tries to
> cherry-pick a merge commit without any -n option).
>
> Of course, what we could do is to introduce a modifier, e.g.
> --cherry-pick=first-parent, that would trigger the old behavior and would
> be asked-for in the --preserve-merges mode.
>
> But quite frankly, personally I would not worry about it *that* much. As
> you pointed out, the patch ID for merge commits is incorrect to begin
> with, and we may just redeclare all merge commits to be incomparable to
> one another when it comes to patch IDs.
>
> In short: I would be fine with the change of behavior.
Thanks for this explanation; it matches what I was thinking, but you
went through it in a lot more detail.
So it sounds like this is the right thing, but as you pointed out, the
implementation is just silly. I'll see if I can come up with a working
v2.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] patch-ids: skip merge commits
From: Jeff King @ 2016-09-07 18:46 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Michael Haggerty, Kevin Willford
In-Reply-To: <alpine.DEB.2.20.1609071338310.129229@virtualbox>
On Wed, Sep 07, 2016 at 02:52:04PM +0200, Johannes Schindelin wrote:
> > diff --git a/patch-ids.c b/patch-ids.c
> > index 77e4663..b1f8514 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 0;
> > diff_tree_sha1(commit->parents->item->object.oid.hash,
> > commit->object.oid.hash, "", options);
> > - else
> > + } else
>
> 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.
> 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.
Thanks, and sorry for the obviously braindead patch.
-Peff
^ permalink raw reply
* Re: [PATCH] t6026-merge-attr: wait for process to release trash directory
From: Junio C Hamano @ 2016-09-07 18:39 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, Johannes Schindelin, Git Mailing List
In-Reply-To: <20160906072707.goysliw32xgwmlol@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Sep 05, 2016 at 09:03:48PM +0200, Johannes Sixt wrote:
>
>> The process spawned in the hook uses the test's trash directory as CWD.
>> As long as it is alive, the directory cannot be removed on Windows.
>> Although the test succeeds, the 'test_done' that follows produces an
>> error message and leaves the trash directory around. Insert a delay to
>> give the hook time to go away.
>
> Ugh. I'd love it if we could avoid inserting a sleep, which wastes time
> in the optimistic case and is insufficient in the pessimistic one.
>
> The fact that the hook is already using sleep is even nastier, as it
> that's a potential race on a loaded system.
>
> Can we do some signaling with fifos to tell the hook when it is safe to
> exit? Then we would just need to `wait` for its parent process.
Is fifo safe on Windows, though?
With v2 that explicitly kills, I guess we can make the sleep longer
without slowing down in the optimistic case?
^ permalink raw reply
* Re: [PATCH] t9903: fix broken && chain
From: Junio C Hamano @ 2016-09-07 18:35 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Git Mailing List
In-Reply-To: <61de943c-df93-58e9-ca48-c469a71a1d43@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> We might wonder why our && chain check does not catch this case:
> The && chain check uses a strange exit code with the expectation that
> the second or later part of a broken && chain would not exit with this
> particular code.
>
> This expectation does not work in this case because __git_ps1, being
> the first command in the second part of the broken && chain, records
> the current exit code, does its work, and finally returns to the caller
> with the recorded exit code. This fools our && chain check.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> t/t9903-bash-prompt.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> index 0db4469..97c9b32 100755
> --- a/t/t9903-bash-prompt.sh
> +++ b/t/t9903-bash-prompt.sh
> @@ -177,7 +177,7 @@ test_expect_success 'prompt - interactive rebase' '
> git checkout b1 &&
> test_when_finished "git checkout master" &&
> git rebase -i HEAD^ &&
> - test_when_finished "git rebase --abort"
> + test_when_finished "git rebase --abort" &&
> __git_ps1 >"$actual" &&
> test_cmp expected "$actual"
> '
Thanks.
^ permalink raw reply
* Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers
From: Junio C Hamano @ 2016-09-07 18:31 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git
In-Reply-To: <20160906184143.55a5zoa2mj6c2e5m@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> What happens to those poor souls on systems without REG_STARTEND? Do
> they get to keep segfaulting?
>
> I think the solution is to push them into setting NO_REGEX. So looking
> at this versus a "regexecn", it seems:
>
> - this lets people keep using their native regexec if it supports
> STARTEND
>
> - this is a bit more clunky to use at the callsites (though we could
> _create_ a portable regexecn wrapper that uses this technique on top
> of the native regex library)
>
> But I much prefer this approach to copying the data just to add a NUL.
I first thought "push them to NO_REGEX" to mean "they live with
crippled Git that does not do regexp" and went "Huh?", but it merely
means "let's avoid platform regex library and use on from the
compat/ hierarchy", which would solve the STARTEND portability issue
for everybody.
Which is very good.
The idea to create a thin regexecn() wrapper also sounds like a good
idea, too. The changes to the callsites in the demonstration patch
does look a bit clunky to me, too.
^ permalink raw reply
* Re: [PATCH 3/4] t5550-http-fetch-dumb.sh: use the GIT_TRACE_CURL environment var
From: Junio C Hamano @ 2016-09-07 18:25 UTC (permalink / raw)
To: Elia Pinto; +Cc: Eric Sunshine, Git List
In-Reply-To: <CA+EOSBki1E8HJ1DMuAuQaB6sBcUjvQLA5xUWdF9G+6+HRQUD7g@mail.gmail.com>
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.
^ permalink raw reply
* Re: [PATCH v1 2/2] read-cache: make sure file handles are not inherited by child processes
From: Junio C Hamano @ 2016-09-07 18:23 UTC (permalink / raw)
To: Eric Wong; +Cc: Lars Schneider, Git Mailing List, tboegi, Johannes.Schindelin
In-Reply-To: <20160907181036.GA14931@starla>
Eric Wong <e@80x24.org> writes:
> We probably should be using O_NOATIME for all O_RDONLY cases
> to get the last bit of performance out (especially since
> non-modern-Linux systems probably still lack relatime).
No, please do not go there.
The user can read from a file in a working tree using "less",
"grep", etc., and they all update the atime, so should "git grep".
We do not use atime ourselves on these files but we should let
outside tools rely on the validity of atime (e.g. "what are the
files that were looked at yesterday?").
If you grep for noatime in our current codebase, you'd notice that
we use it only for files in objects/ hierarchy, and that makes very
good sense. These files are what we create for our _sole_ use and
no other tools can peek at them and expect to get any useful
information out of them (we hear from time to time that virus
scanners leaving open file descriptors on them causing trouble, but
that is an example of a useless access), and that makes a file in
objects/ hierarchy a fair game for noatime optimization.
^ permalink raw reply
* Re: [PATCH v1 1/2] sha1_file: open window into packfiles with CLOEXEC
From: Eric Wong @ 2016-09-07 18:17 UTC (permalink / raw)
To: Lars Schneider; +Cc: Johannes Schindelin, git, gitster, tboegi
In-Reply-To: <F8E7B7CE-1177-4CBD-999E-21C593A8ACD2@gmail.com>
Lars Schneider <larsxschneider@gmail.com> wrote:
> > On 06 Sep 2016, at 13:38, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> > On Mon, 5 Sep 2016, Eric Wong wrote:
> >> larsxschneider@gmail.com wrote:
> >>> -int git_open_noatime(const char *name)
> >>> +int git_open_noatime_cloexec(const char *name)
> >>> {
> >>> - static int sha1_file_open_flag = O_NOATIME;
> >>> + static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;
> >>>
> >>> for (;;) {
> >>> int fd;
> >
> >> I question the need for the "_cloexec" suffixing in the
> >> function name since the old function is going away entirely.
> >
> > Me, too. While it is correct, it makes things harder to read, so it may
> > even cause more harm than it does good.
>
> What name would you suggest? Leaving the name as-is seems misleading to me.
> Maybe just "git_open()" ?
Maybe "_noatime" is useful in some cases, but maybe not *shrug*
My original point for removing the "_cloexec" suffix was that
(at least for Perl and Ruby), cloexec-by-default was so prevalent
in FD-creating syscalls that having the suffix wasn't needed.
> >> I prefer all FD-creating functions set cloexec by default
> >> for FD > 2 to avoid inadvertantly leaking FDs. So we
> >> ought to use pipe2, accept4, socket(..., SOCK_CLOEXEC), etc...
> >> and fallback to the racy+slower F_SETFD when not available.
> I applied the same mechanism here. Would that be OK?
>
> Thanks,
> Lars
>
> - static int sha1_file_open_flag = O_NOATIME;
> + static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;
>
> for (;;) {
> int fd;
> @@ -1471,12 +1471,17 @@ int git_open_noatime(const char *name)
> if (fd >= 0)
> return fd;
>
> - /* Might the failure be due to O_NOATIME? */
> - if (errno != ENOENT && sha1_file_open_flag) {
> - sha1_file_open_flag = 0;
> + /* Try again w/o O_CLOEXEC: the kernel might not support it */
> + if (O_CLOEXEC && errno == EINVAL && (sha1_file_open_flag & O_CLOEXEC)) {
80 columns overflow
> + sha1_file_open_flag &= ~O_CLOEXEC;
> continue;
> }
>
> + /* Might the failure be due to O_NOATIME? */
> + if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {
> + sha1_file_open_flag &= ~O_NOATIME;
> + continue;
> + }
But otherwise much better since it doesn't blindly zero
sha1_file_open_flag :>
^ permalink raw reply
* Re: [PATCH v3 0/8] Better heuristics make prettier diffs
From: Junio C Hamano @ 2016-09-07 18:11 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:
> * 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.
^ permalink raw reply
* Re: [PATCH v1 2/2] read-cache: make sure file handles are not inherited by child processes
From: Eric Wong @ 2016-09-07 18:10 UTC (permalink / raw)
To: Lars Schneider; +Cc: Git Mailing List, gitster, tboegi, Johannes.Schindelin
In-Reply-To: <7B903664-0324-4375-A81C-1317020CBE9B@gmail.com>
Lars Schneider <larsxschneider@gmail.com> wrote:
> > On 06 Sep 2016, at 23:06, Eric Wong <e@80x24.org> wrote:
> > larsxschneider@gmail.com wrote:
> >> static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
> >> {
> >> int match = -1;
> >> - int fd = open(ce->name, O_RDONLY);
> >> + int fd = open(ce->name, O_RDONLY | O_CLOEXEC);
> >>
> >> if (fd >= 0) {
> >> unsigned char sha1[20];
> >
> > Also, this needs to check EINVAL when O_CLOEXEC != 0 the same
> > way create_tempfile currently does. Somebody could be building
> > with modern headers but running an old kernel that doesn't
> > understand O_CLOEXEC.
> >
> > There should probably be a open() wrapper for handling this case
> > since we're now up to 3 places where open(... O_CLOEXEC) is
> > used.
>
> Right! Actually "sha1_file.c:git_open_noatime()" is already a wrapper, no?
> Can't we use this here? The O_NOATIME flag shouldn't hurt, right?
For ce_compare_data (and other O_RDONLY users), I guess
git_open_noatime is fine; and probably preferable because of
O_NOATIME.
We probably should be using O_NOATIME for all O_RDONLY cases
to get the last bit of performance out (especially since
non-modern-Linux systems probably still lack relatime).
However, create_tempfile needs O_RDWR|O_CREAT|O_EXCL
but I guess we can clean that up in another series.
^ permalink raw reply
* Re: [PATCHv3] diff.c: emit moved lines with a different color
From: Stefan Beller @ 2016-09-07 18:02 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jacob Keller, Stefan Beller, Git mailing list,
Jakub Narębski
In-Reply-To: <xmqq7fanoas6.fsf@gitster.mtv.corp.google.com>
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.
^ permalink raw reply
* Re: [PATCH v2 7/7] blame: actually use the diff opts parsed from the command line
From: Junio C Hamano @ 2016-09-07 17:58 UTC (permalink / raw)
To: Michael Haggerty
Cc: René Scharfe, git, Stefan Beller, Jeff King,
Jakub Narębski, Jacob Keller
In-Reply-To: <a6d9e9a7-76c4-5b26-0b49-55698369180d@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> The reason that I would prefer to change `blame` as part of this patch
> series is that I think it would be disconcerting for `git diff` and `git
> blame` to use different heuristics when computing diffs. It would make
> their output inconsistent.
I do think it is the right thing to do. With your shifting heuristics,
"git diff" would attribute an addition of a whole block more
correctly, e.g.
}
+foo {
+ bar
+ baz
+}
instead of attributing the tail of the new thing to the old author,
and the "blame" should take advantage of the better heuristics as
well.
^ permalink raw reply
* Re: [PATCHv3] diff.c: emit moved lines with a different color
From: Junio C Hamano @ 2016-09-07 17:54 UTC (permalink / raw)
To: Stefan Beller
Cc: Jacob Keller, Stefan Beller, Git mailing list,
Jakub Narębski
In-Reply-To: <CAGZ79ka7VV6Pqw-rcVFv0xU4J4D9dHWPg-oocPPWJdY5JVhT+A@mail.gmail.com>
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?
^ permalink raw reply
* Re: [PATCH] stash: allow ref of a stash by index
From: Junio C Hamano @ 2016-09-07 17:50 UTC (permalink / raw)
To: Øystein Walle
Cc: Jeff King, Aaron M Watson, Git, Jon Seymour, David Caldwell,
Ævar Arnfjörð Bjarmason, David Aguilar,
Alex Henrie
In-Reply-To: <CAFaJEqu-JUcwLjrQBk_huSa3DZfCf8O4eAZ=UgcXHzN=CLgtpw@mail.gmail.com>
Øystein Walle <oystwa@gmail.com> writes:
> diff --git a/git-stash.sh b/git-stash.sh
> index 826af18..b026288 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -384,7 +384,7 @@ parse_flags_and_rev()
> i_tree=
> u_tree=
>
> - REV=$(git rev-parse --no-flags --symbolic --sq "$@") || exit 1
> + REV=$(git rev-parse --no-flags --symbolic --sq "$@" 2>/dev/null)
>
> FLAGS=
> for opt
> @@ -422,6 +422,15 @@ parse_flags_and_rev()
> ;;
> esac
>
> + case "$1" in
> + *[!0-9]*)
> + :
OK, so you ignore anything that has a non-digit here, to ensure that...
> + ;;
> + *)
> + set -- "${ref_stash}@{$1}"
... this one triggers only for a string $1 that consists solely of
digits.
> + ;;
> + esac
Makes sense. I notice that both of the two existing case/esac
statements in this function indent case arms and their bodies one
level too deep, which you follwed with the above addition. That may
be something we would want to fix in a follow-up patch.
> REV=$(git rev-parse --symbolic --verify --quiet "$1") || {
> reference="$1"
> die "$(eval_gettext "\$reference is not a valid reference")"
I agree with Peff that the change in error message he noticed is
probably an improvement ;-)
Want to do a final log message to make it a real patch?
Thanks.
^ permalink raw reply
* Re: [PATCH] compat: move strdup(3) replacement to its own file
From: Junio C Hamano @ 2016-09-07 17:44 UTC (permalink / raw)
To: René Scharfe; +Cc: Johannes Schindelin, Git List
In-Reply-To: <6926b39c-4448-c463-33f7-d9eae841c635@web.de>
René Scharfe <l.s.r@web.de> writes:
> Well, OK. I think the missing point is that the original nedmalloc
> doesn't come with strdup() and doesn't need it. Only _users_ of
> nedmalloc need it. Marius added it in nedmalloc.c, but strdup.c is a
> better place for it.
Thanks. I'll add these lines like so:
Move our implementation of strdup(3) out of compat/nedmalloc/ and
allow it to be used independently from USE_NED_ALLOCATOR. The
original nedmalloc doesn't come with strdup() and doesn't need it.
Only _users_ of nedmalloc need it, which was added when we imported
it to our compat/ hierarchy.
This reduces the difference of our copy of nedmalloc from the
original, making it easier to update, and allows for easier testing
and reusing of our version of strdup().
^ permalink raw reply
* Re: [PATCH v2 6/6] git-gui: Update Japanese information
From: Junio C Hamano @ 2016-09-07 17:35 UTC (permalink / raw)
To: Satoshi Yasushima, Pat Thoyts; +Cc: git, Jakub Narębski
In-Reply-To: <048AF5D4DC044BDC9F3A75A3BABD00F1@Yasushima>
"Satoshi Yasushima" <s.yasushima@gmail.com> writes:
> There seems to be a cause in the following taboo.
> Sure, PATCH 3/6 is little too great.
> http://vger.kernel.org/majordomo-info.html#taboo
> >Taboo things to be done when discussing at VGER lists
> : (abbr.)
> > * Message size exceeding 100 000 characters causes blocking.
>
> To somehow.
Since I received the patch directly bypassing vger, I queued it on
gitgui-0.20.0 from Pat and tentatively merged it to my 'pu'.
Pat, we haven't heard from you for a long time. How would you want
to proceed? If you fetch from me and merge 52285c83 ("git-gui:
update Japanese information", 2016-09-07), you'd get these six
commits for po/ja.po and then we'd be in sync next time I pull from
you.
That is, if that ever happens--is git-gui pretty much in deep
maintenance mode without anything more to do from your point of
view?
Thanks.
^ permalink raw reply
* Re: [PATCH v2 2/2] connect: advertized capability is not a ref
From: Junio C Hamano @ 2016-09-07 17:10 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, jrnieder, spearce, sbeller, peff
In-Reply-To: <174c8ca6638f1cd3145a628925e65655b56af366.1472853827.git.jonathantanmy@google.com>
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).
> +
> if (!check_ref(name, flags))
> continue;
> ref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);
^ permalink raw reply
* Re: [PATCH v2] rebase -i: improve advice on bad instruction lines
From: Ralf Thielow @ 2016-09-07 17:07 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: git, Junio C Hamano
In-Reply-To: <1473240491.20551.1.camel@kaarsemaker.net>
2016-09-07 11:28 GMT+02:00 Dennis Kaarsemaker <dennis@kaarsemaker.net>:
> Hi Ralf,
>
> There are quite a few patch series in flight these days around
> interactive rebase. Have you checked for conflicts with those?
>
Thanks. I did not check against 'pu' when I created this patch but I'm able
to apply without any conflicts.
^ permalink raw reply
* Re: [PATCH v2 2/2] connect: advertized capability is not a ref
From: Junio C Hamano @ 2016-09-07 17:02 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, jrnieder, spearce, sbeller, peff
In-Reply-To: <174c8ca6638f1cd3145a628925e65655b56af366.1472853827.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> Git advertises the same capabilities^{} ref in its ref advertisement for push
> but since it never remembered to do so for fetch, the client forgot to handle
> this case. Handle it.
> ...
> In this aspect, JGit is compliant with the specification in pack-protocol.txt.
I agree with Peff that the above explanation distorts the history.
It should be reworded.
I do not have an issue with being more lenient to what JGit servers
have been doing for a few years, though.
> Because git client versions without this fix are expected to exist in the wild
> for a while, we should not change the server to always send the capabilities^{}
> line when there are no refs to advertise yet. A transition will take multiple
> steps:
>
> 1. This patch, which updates the client
>
> 2. Update pack-protocol to clarify that both server behaviors must be
> tolerated.
>
> 3. Add a configuration variable to allow git upload-pack to advertise
> capabilities when there are no refs to advertise. Leave it disabled
> by default since git clients can't be counted on to have this patch (1)
> yet.
>
> 4. After a year or so, flip the default for that server configuration
> variable to true.
The above assumes it is an unconditionally good thing to send
capabilities^{}; I do not think we established that in this
discussion, and more importantly, this client-side change is a good
thing to do regardless of the outcome of the discussion.
I'd suggest dropping everything below "Because Git client versions
without...".
> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> index 819b9dd..2de52f5 100755
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
> @@ -207,5 +207,44 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
> test_cmp expect actual
> '
>
> +test_lazy_prereq GIT_DAEMON '
> + test_tristate GIT_TEST_GIT_DAEMON &&
> + test "$GIT_TEST_GIT_DAEMON" != false
> +'
> +
> +JGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}}
Shouldn't this be inside the next expect_success?
> +# This test spawns a daemon, so run it only if the user would be OK with
> +# testing with git-daemon.
> +test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '
> + JGIT_DAEMON_PID= &&
> + git init --bare empty.git &&
> + touch empty.git/git-daemon-export-ok &&
To make it clear that the existence of the file is the thing you
care the most, not that the file having a recent timestamp:
>empty.git/git-daemon-export-ok &&
> + mkfifo jgit_daemon_output &&
> + {
> + jgit daemon --port="$JGIT_DAEMON_PORT" . >jgit_daemon_output &
> + JGIT_DAEMON_PID=$!
> + } &&
> + test_when_finished kill "$JGIT_DAEMON_PID" &&
> + {
> + read line &&
> + case $line in
> + Exporting*)
> + ;;
> + *)
> + echo "Expected: Exporting" &&
> + false;;
> + esac &&
> + read line &&
> + case $line in
> + "Listening on"*)
> + ;;
> + *)
> + echo "Expected: Listening on" &&
> + false;;
> + esac
OK, so this is a nice way to wait until the daemon becomes ready to
serve.
> + } <jgit_daemon_output &&
An in-code comment that explains what the significance of "2" here
# --exit-code asks the command to exit with 2 when no
# matching refs are found.
would be nice.
> + test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git
> +'
Thanks.
^ 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