* Re: ERANGE strikes again on my Windows build; RFH
From: Johannes Sixt @ 2019-12-29 18:08 UTC (permalink / raw)
To: Alban Gruin, Torsten Bögershausen; +Cc: Git Mailing List
In-Reply-To: <a32e075b-9e6c-2b6a-8619-0330501eee97@gmail.com>
Am 29.12.19 um 18:25 schrieb Alban Gruin:
> Le 29/12/2019 à 15:29, Torsten Bögershausen a écrit :
>> On Sat, Dec 28, 2019 at 04:41:42PM +0100, Johannes Sixt wrote:
>>> In sha1-file.c:read_object_file_extended() we have the following pattern:
>>>
>>> errno = 0;
>>> data = read_object(r, repl, type, size);
>>> if (data)
>>> return data;
>>>
>>> if (errno && errno != ENOENT)
>>> die_errno(_("failed to read object %s"), oid_to_hex(oid));
>>>
>>> That is, it is expected that read_object() does not change the value of
>>> errno in the non-error case. I find it intriguing that we expect a quite
>>> large call graph that is behind read_object() to behave this way.
>>>
>>> What if a subordinate callee starts doing
>>>
>>> if (some_syscall(...) < 0) {
>>> if (errno == EEXIST) {
>>> /* never mind, that's OK */
>>> ...
>>> }
>>> }
>>>
>>> Would it be required to reset errno to its previous value in this
>>> failure-is-not-an-error case?
>>>
>>> The problem in my Windows build is that one of these subordinate
>>> syscalls is vsnprintf() (as part of a strbuf_add variant, I presume),
>>> and it fails with ERANGE when the buffer is too short. Do I have to
>>> modify the vsnprintf emulation to restore errno?
>>
>> If you ask me: I think so, yes.
>> At least the documentation about vsnprintf does not mention that errno is touched at all.
>> That is the man pages for Linux and Mac OS, or see here:
>> https://linux.die.net/man/3/vsnprintf
>>
>> It would make sense to analyze the complete callstack, I think.
>> Is your problem reproducable ?
>>
>> Changing the function strbuf_vaddf() strbuf.c seems to be straight forward to me.
>>
>
> According to the standard, vsnprintf() _can_ change errno[1] (and the
> BSDs do so[2][3][4].) But apparently, not to ERANGE.
I am not worried about errno being set (or to what value) when there
actually is an error. I am asking what to do when there is actually *no*
error. In my vsnprintf emulation, the case where ERANGE happens is *not*
an error as far as the emulation is concerned.
What if in the huge call graph behind read_object() some function
changes errno to, say, EEXIST, EISDIR, or ENODIR and the condition under
which this happens is *not* an error in that context? Is the function
required to restore the original errno?
Consider the task to create file "foo/bar.c". We would have to
mkdir("foo"), but it is *not* an error when mkdir() fails with errno ==
EEXIST. Are we required to reset errno back to its old value?
(I know, read_object() is unlikely to allocate files, but I think I have
to explain in some way that the context may define that there is no
error -- even though a lower-level function failed and modified errno.)
-- Hannes
^ permalink raw reply
* Re: git filters don't get applied to dotfiles
From: Dennis Kaarsemaker @ 2019-12-29 16:01 UTC (permalink / raw)
To: Adrien LEMAIRE, git
In-Reply-To: <CALqn52MbeiCrEzphMkcjeU6bPbLLaQOa-vzht2156uqVw1wL_g@mail.gmail.com>
On Fri, 2019-12-27 at 16:51 +0900, Adrien LEMAIRE wrote:
> I'd like to report a bug regarding git filters not being applied to
> files beginning with a dot character "."
> Using git version 2.24.1
> Please let me know if there is a better way to report bugs. The github
> page only mentions this email.
<snip reproduction recipe>
I was not able to reproduce this in the git test suite with a quick
patch (see below). Your output does not show any git add command, is it
possible that you added the changes before configuring the filter?
If you set GIT_TRACE=2 in your environment before doing the git add of
the .mailrc file, you should see it run the filter command. It should
look something like:
+ git add test test.t test.i .mailrc
trace: built-in: git add test test.t test.i .mailrc
trace: run_command: ./rot13.sh
trace: run_command: ./rot13.sh
(which is a part of the output of GIT_TRACE=2 ./t0021-conversion.sh -x
-v -i)
diff --git t/t0021-conversion.sh t/t0021-conversion.sh
index 6c6d77b51a..32c27d513b 100755
--- t/t0021-conversion.sh
+++ t/t0021-conversion.sh
@@ -77,6 +77,7 @@ test_expect_success setup '
{
echo "*.t filter=rot13"
+ echo ".mailrc filter=rot13"
echo "*.i ident"
} >.gitattributes &&
@@ -88,9 +89,10 @@ test_expect_success setup '
cat test >test.t &&
cat test >test.o &&
cat test >test.i &&
- git add test test.t test.i &&
+ cat test >.mailrc &&
+ git add test test.t test.i .mailrc &&
rm -f test test.t test.i &&
- git checkout -- test test.t test.i &&
+ git checkout -- test test.t test.i .mailrc &&
echo "content-test2" >test2.o &&
echo "content-test3 - filename with special characters" >"test3 '\''sq'\'',\$x=.o"
@@ -102,6 +104,7 @@ test_expect_success check '
test_cmp test.o test &&
test_cmp test.o test.t &&
+ test_cmp test.o .mailrc &&
# ident should be stripped in the repository
git diff --raw --exit-code :test :test.i &&
@@ -110,9 +113,12 @@ test_expect_success check '
test "z$id" = "z$embedded" &&
git cat-file blob :test.t >test.r &&
+ git cat-file blob :.mailrc >.mailrc.r &&
./rot13.sh <test.o >test.t &&
- test_cmp test.r test.t
+ ./rot13.sh <test.o >.mailrc &&
+ test_cmp test.r test.t &&
+ test_cmp .mailrc.r .mailrc
'
# If an expanded ident ever gets into the repository, we want to make sure that
^ permalink raw reply related
* [PATCH 1/1] diff-highlight: highlight range-diff
From: Jack Bates via GitGitGadget @ 2019-12-29 15:49 UTC (permalink / raw)
To: git; +Cc: Jack Bates, Junio C Hamano, Jack Bates
In-Reply-To: <pull.684.git.git.1577634590.gitgitgadget@gmail.com>
From: Jack Bates <jack@nottheoilrig.com>
Piping `git range-diff` through diff-highlight currently has no effect,
for two reasons:
1. There are ANSI escapes before and after the `@@` hunk headers (when
color is enabled) which diff-highlight fails to match. One solution
is to match both escapes (`/^$COLOR*\@\@$COLOR* /`). This patch
drops the trailing space from the existing pattern instead.
2. Unlike `git log`, `git range-diff` diffs are indented, which
diff-highlight also fails to match. This patch allows hunk headers
preceded by any amount of whitespace, and then skips past that
indentation when parsing subsequent lines, by reusing the machinery
that handles the --graph output.
Signed-off-by: Jack Bates <jack@nottheoilrig.com>
---
contrib/diff-highlight/DiffHighlight.pm | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm
index e2589922a6..74f53e53c9 100644
--- a/contrib/diff-highlight/DiffHighlight.pm
+++ b/contrib/diff-highlight/DiffHighlight.pm
@@ -90,7 +90,8 @@ sub handle_line {
if (!$in_hunk) {
$line_cb->($orig);
- $in_hunk = /^$COLOR*\@\@ /;
+ $in_hunk = /^( *)$COLOR*\@\@/;
+ $graph_indent = length($1);
}
elsif (/^$COLOR*-/) {
push @removed, $orig;
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/1] diff-highlight: highlight range-diff
From: Jack Bates via GitGitGadget @ 2019-12-29 15:49 UTC (permalink / raw)
To: git; +Cc: Jack Bates, Junio C Hamano
Piping git range-diff through diff-highlight currently has no effect, for
two reasons:
1. There are ANSI escapes before and after the @@ hunk headers (when color
is enabled) which diff-highlight fails to match. One solution is to
match both escapes (/^$COLOR*\@\@$COLOR* /). This patch drops the
trailing space from the existing pattern instead.
2. Unlike git log, git range-diff diffs are indented, which diff-highlight
also fails to match. This patch allows hunk headers preceded by any
amount of whitespace, and then skips past that indentation when parsing
subsequent lines, by reusing the machinery that handles the --graph
output.
Signed-off-by: Jack Bates jack@nottheoilrig.com [jack@nottheoilrig.com]
Jack Bates (1):
diff-highlight: highlight range-diff
contrib/diff-highlight/DiffHighlight.pm | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-684%2Fjablko%2Fpatch-1-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-684/jablko/patch-1-v1
Pull-Request: https://github.com/git/git/pull/684
--
gitgitgadget
^ permalink raw reply
* Re: unsub
From: Kevin Daudt @ 2019-12-29 14:58 UTC (permalink / raw)
To: Lionel Dyck; +Cc: Git Mailing List
In-Reply-To: <SN6PR03MB3695BF61648C995A5124A0D2B8240@SN6PR03MB3695.namprd03.prod.outlook.com>
On Sun, Dec 29, 2019 at 02:44:34PM +0000, Lionel Dyck wrote:
>
> unsubscribe
To acually unsubscribe from this mailing list, you need to send an
e-mail to majordomo@vger.kernel.org, not to this mailing list with the
body 'unsubscribe git'.
Kind regards, Kevin
^ permalink raw reply
* unsub
From: Lionel Dyck @ 2019-12-29 14:44 UTC (permalink / raw)
To: Git Mailing List
unsubscribe
^ permalink raw reply
* Re: ERANGE strikes again on my Windows build; RFH
From: Andreas Schwab @ 2019-12-29 14:43 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Johannes Sixt, Git Mailing List
In-Reply-To: <20191229142909.7bmjbrroboitvnzq@tb-raspi4>
On Dez 29 2019, Torsten Bögershausen wrote:
> At least the documentation about vsnprintf does not mention that errno is touched at all.
> That is the man pages for Linux and Mac OS, or see here:
> https://linux.die.net/man/3/vsnprintf
Refer to the POSIX docs for vsnprintf, which documents its errno usage.
In general, any library call can touch errno, unless it is explicitly
documented _not_ to touch it.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply
* Re: ERANGE strikes again on my Windows build; RFH
From: Torsten Bögershausen @ 2019-12-29 14:29 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Git Mailing List
In-Reply-To: <6eb02a73-9dcb-f1fc-f015-80e71e9910d6@kdbg.org>
On Sat, Dec 28, 2019 at 04:41:42PM +0100, Johannes Sixt wrote:
> In sha1-file.c:read_object_file_extended() we have the following pattern:
>
> errno = 0;
> data = read_object(r, repl, type, size);
> if (data)
> return data;
>
> if (errno && errno != ENOENT)
> die_errno(_("failed to read object %s"), oid_to_hex(oid));
>
> That is, it is expected that read_object() does not change the value of
> errno in the non-error case. I find it intriguing that we expect a quite
> large call graph that is behind read_object() to behave this way.
>
> What if a subordinate callee starts doing
>
> if (some_syscall(...) < 0) {
> if (errno == EEXIST) {
> /* never mind, that's OK */
> ...
> }
> }
>
> Would it be required to reset errno to its previous value in this
> failure-is-not-an-error case?
>
> The problem in my Windows build is that one of these subordinate
> syscalls is vsnprintf() (as part of a strbuf_add variant, I presume),
> and it fails with ERANGE when the buffer is too short. Do I have to
> modify the vsnprintf emulation to restore errno?
If you ask me: I think so, yes.
At least the documentation about vsnprintf does not mention that errno is touched at all.
That is the man pages for Linux and Mac OS, or see here:
https://linux.die.net/man/3/vsnprintf
It would make sense to analyze the complete callstack, I think.
Is your problem reproducable ?
Changing the function strbuf_vaddf() strbuf.c seems to be straight forward to me.
^ permalink raw reply
* Re: Re: [PATCH 1/1] git-gui: add possibility to open currently selected file
From: Zoli Szabó @ 2019-12-29 13:23 UTC (permalink / raw)
To: Pratyush Yadav; +Cc: git
In-Reply-To: <20191227193418.36uzeizs37nv7ywb@yadavpratyush.com>
Hi Pratyush,
Thanks for your thorough review.
On 2019.12.27 21:34, Pratyush Yadav wrote:
>> ...This change enables just that by:
>> - Diff header path context menu -> Open;
>
> Would it be a better idea to have this option in the diff body context
> menu (.vpane.lower.diff.body.ctxm) instead? The problem I see with the
> way its currently done is visibility/discovery. It is not very likely
> for a user to try and click the file name which doesn't give any
> indication that it is clickable. So how will someone who hasn't read
> this commit message know that they can use this neat feature. The diff
> body context menu is much more "visible" IMO.
>
>> - or double-clicking the diff header path.
>
> An alternative to the above suggestion would be to make this path
> underlined and blue in color (like a hyperlink in a web browser). This
> will give the indication that this is not just plain text.
>
> I like the latter idea more, but I don't mind either.
For me, the body context menu holds the diff-related options, I am not
sure the "Open" fits in there. But I do like your suggestion of making
the filename web browser-link-like. I'll try to implement that.
>> One "downside" of the approach is that executable files will be run
>> and not opened for editing.
>
> FWIW, I do not see it as a downside at all. The menu option is called
> "open" not "edit". So if you click it, you should expect the file to
> open. In case its a binary file, executing it is the correct outcome. In
> case its a text file, opening it in the editor is the correct outcome.
Alright then.
>> +proc do_file_open {file} {
>> + global _gitworktree
>> + set explorer [get_explorer]
>> + set full_file_path [file join $_gitworktree $file]
>> + eval exec $explorer [list [file nativename $full_file_path]] &
>
> This executes $explorer, which is 'explorer.exe' on Windows. I'm not a
> heavy Windows user but AFAIK it is a file manager. This makes it quite
> different from 'xdg-open' which is used to open _any_ file/URL in the
> user's default application. So it also happens to open _directories_ in
> the default file explorer which was the original intention of this
> procedure.
>
> Have you tested it on Windows? Does 'explorer.exe' do the correct thing?
>
> Looking at MacOS's 'open' man page, I think it should also work like
> xdg-open and shouldn't be a problem.
I am in fact working on this patch on Windows. explorer.exe works also
for files and even brings up the "Open with..." dialog for
not-recognized file types.
> Tested on Linux. Works fine. Looking forward to the re-roll.
Thanks for testing it on Linux. Since I am working on Windows, I just
read about `xdg-open` and `open` and assumed everything should work OK.
All other points I agree with you and will push a new version of the patch.
Thanks,
Zoli
^ permalink raw reply
* Re: [PATCH 0/9] [RFC] Changed Paths Bloom Filters
From: Jeff King @ 2019-12-29 6:24 UTC (permalink / raw)
To: Derrick Stolee
Cc: Garima Singh via GitGitGadget, git, szeder.dev, jonathantanmy,
jeffhost, me, Junio C Hamano
In-Reply-To: <e9a4e4ff-5466-dc39-c3f5-c9a8b8f2f11d@gmail.com>
On Fri, Dec 27, 2019 at 11:11:37AM -0500, Derrick Stolee wrote:
> > So here are a few patches to reduce the CPU and memory usage. They could
> > be squashed in at the appropriate spots, or perhaps taken as inspiration
> > if there are better solutions (especially for the first one).
>
> I tested these patches with the Linux kernel repo and reported my results
> on each patch. However, I wanted to also test on a larger internal repo
> (the AzureDevOps repo), which has ~500 commits with more than 512 changes,
> and generally has larger diffs than the Linux kernel repo.
>
> | Version | Time | Memory |
> |----------|--------|--------|
> | Garima | 16m36s | 27.0gb |
> | Peff 1 | 6m32s | 28.0gb |
> | Peff 2 | 6m48s | 5.6gb |
> | Peff 3 | 6m14s | 4.5gb |
> | Shortcut | 3m47s | 4.5gb |
>
> For reference, I found the time and memory information using
> "/usr/bin/time --verbose" in a bash script.
Thanks for giving it more exercise. My heap profiling was done with
massif, which measures the heap directly. Measuring RSS would cover
that, but will also include the mmap'd packfiles. That's probably why
your linux.git numbers were slightly higher than mine.
(massif is a really great tool if you haven't used it, as it also shows
which allocations were using the memory. But it's part of valgrind, so
it definitely doesn't run on native Windows. It might work under WSL,
though. I'm sure there are also other heap profilers on Windows).
> By "Shortcut" in the table above, I mean the following patch on top of
> Garima's and Peff's changes. It inserts a max_changes option into struct
> diff_options to halt the diff early. This seemed like an easier change
> than creating a new tree diff algorithm wholesale.
Yeah, I'm not opposed to a diff feature like this.
But be careful, because...
> diff --git a/diff.h b/diff.h
> index 6febe7e365..9443dc1b00 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -285,6 +285,11 @@ struct diff_options {
> /* Number of hexdigits to abbreviate raw format output to. */
> int abbrev;
>
> + /* If non-zero, then stop computing after this many changes. */
> + int max_changes;
> + /* For internal use only. */
> + int num_changes;
This is holding internal state in diff_options, but the same
diff_options is often used for multiple diffs (e.g., "git log --raw"
would use the same rev_info.diffopt over and over again).
So it would need to be cleared between diffs. There's a similar feature
in the "has_changes" flag, though it looks like it is cleared manually
by callers. Yuck.
This isn't a problem for commit-graph right now, but:
- it actually could be using a single diff_options, which would be
slightly simpler (it doesn't seem to save much CPU, though, because
the initialization is relatively cheap)
- it's a bit of a subtle bug to leave hanging around for the next
person who tries to use the feature
I actually wonder if this could be rolled into the has_changes and
diff_can_quit_early() feature. This really just a generalization of that
feature (which is like setting max_changes to "1").
-Peff
^ permalink raw reply
* Re: [PATCH 1/3] commit-graph: examine changed-path objects in pack order
From: Jeff King @ 2019-12-29 6:28 UTC (permalink / raw)
To: Derrick Stolee
Cc: Garima Singh via GitGitGadget, git, szeder.dev, jonathantanmy,
jeffhost, me, Junio C Hamano
In-Reply-To: <20191229061246.GB220034@coredump.intra.peff.net>
On Sun, Dec 29, 2019 at 01:12:46AM -0500, Jeff King wrote:
> Yeah, I expected that would cover it, too. But instrumenting it to dump
> the position of each commit (see patch below), and then decorating "git
> log" output with the positions (see script below) shows that we're all
> over the map:
I forgot the patch, of course. :)
I just dumped this trace:
---
diff --git a/commit-graph.c b/commit-graph.c
index a6c4ab401e..1cb77be45f 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -61,6 +61,7 @@ static void set_commit_pos(struct repository *r, const struct object_id *oid)
if (!commit)
return; /* should never happen, but be lenient */
+ trace_printf("pos %s = %d", oid_to_hex(oid), max_pos);
*commit_pos_at(&commit_pos, commit) = max_pos++;
}
with:
rm -f .git/objects/info/commit-graph
GIT_TRACE=$PWD/trace.out git commit-graph write --changed-paths --reachable
and then used:
cat >foo.pl <<\EOF
#!/usr/bin/perl
my %deco = do {
open(my $fh, '<', 'trace.out');
map { /pos (\S+) = (\d+)/ ? ($1 => $2) : () } <$fh>
};
while (<>) {
s/([0-9a-f]{40})/$deco{$1}/;
print;
}
EOF
like so:
git log --graph --format=%H |
perl foo.pl |
less
-Peff
^ permalink raw reply related
* Re: [PATCH 1/3] commit-graph: examine changed-path objects in pack order
From: Jeff King @ 2019-12-29 6:12 UTC (permalink / raw)
To: Derrick Stolee
Cc: Garima Singh via GitGitGadget, git, szeder.dev, jonathantanmy,
jeffhost, me, Junio C Hamano
In-Reply-To: <8b331ef6-f431-56ef-37a9-1d6e263ea0fe@gmail.com>
On Fri, Dec 27, 2019 at 09:51:02AM -0500, Derrick Stolee wrote:
> On 12/22/2019 4:32 AM, Jeff King wrote:
> > Looking at the diff of commit objects in pack order is much faster than
> > in sha1 order, as it gives locality to the access of tree deltas
> > (whereas sha1 order is effectively random). Unfortunately the
> > commit-graph code sorts the commits (several times, sometimes as an oid
> > and sometimes a pointer-to-commit), and we ultimately traverse in sha1
> > order.
> >
> > Instead, let's remember the position at which we see each commit, and
> > traverse in that order when looking at bloom filters. This drops my time
> > for "git commit-graph write --changed-paths" in linux.git from ~4
> > minutes to ~1.5 minutes.
>
> I'm doing my own perf tests on these patches, and my copy of linux.git
> has four packs of varying sizes (corresponding with my rare fetches and
> lack of repacks). My time goes from 3m50s to 3m00s. I was confused at
> first, but then realized that I used the "--reachable" flag. In that
> case, we never run set_commit_pos(), so all positions are equal and the
> sort is not helpful.
>
> I thought that inserting some set_commit_pos() calls into close_reachable()
> and add_missing_parents() would give some amount of time-order to the
> commits as we compute the filters. However, the time did not change at
> all.
>
> I've included the patch below for reference, anyway.
Yeah, I expected that would cover it, too. But instrumenting it to dump
the position of each commit (see patch below), and then decorating "git
log" output with the positions (see script below) shows that we're all
over the map:
* 3
|\
| * 2791
| * 5476
| * 8520
| * 12040
| * 16036
* | 2790
|\ \
| * | 5475
| * | 8519
| * | 12039
| * | 16035
| * | 20517
| * | 25527
| |/
* | 5474
|\ \
| * | 8518
| * | 12038
* | | 8517
[...]
I think the root issue is that we never do any date-sorting on the
commits. So:
- we hit each ref tip in lexical order; with tags, this is quite often
the opposite of reverse-chronological
- we traverse breadth-first, but we don't order queue at all. So if we
see a merge X, then we'll next process X^1 and X^2, and then X^1^,
and then X^2^, and so forth. So we keep digging equally down
simultaneous branches, even if one branch is way shorter than the
other. Whereas a regular Git traversal will order the queue by
commit timestamp, so it tends to be roughly chronological (of course
a topo-sort would work too, but that's probably overkill).
I wonder if this would be simpler if "commit-graph --reachable" just
used the regular revision machinery instead of doing its own custom
traversal.
-Peff
^ permalink raw reply
* Re: [PATCH 0/9] [RFC] Changed Paths Bloom Filters
From: Jeff King @ 2019-12-29 6:03 UTC (permalink / raw)
To: Derrick Stolee
Cc: Garima Singh via GitGitGadget, git, szeder.dev, jonathantanmy,
jeffhost, me, Junio C Hamano
In-Reply-To: <fc30441a-1bb7-73e5-43f6-6e26824e04f6@gmail.com>
On Thu, Dec 26, 2019 at 09:21:36AM -0500, Derrick Stolee wrote:
> Here are some recommendations (to Garima) for how to proceed with these
> patches. Please let me know if anyone disagrees.
>
> > [1/3]: commit-graph: examine changed-path objects in pack order
>
> This one is best kept as its own patch, as it shows a clear reason why
> we want to do the sort-by-position. It would also be a complicated
> patch to include this logic along with the first use of
> compute_bloom_filters().
Yeah, I'd agree this one could be a separate patch. It does need more
work, though (as you found out, it does not cover --reachable at all).
The position counter also probably ought to be an unsigned (or even a
uint32_t, which we usually consider a maximum bound for number of
objects).
-Peff
^ permalink raw reply
* ERANGE strikes again on my Windows build; RFH
From: Johannes Sixt @ 2019-12-28 15:41 UTC (permalink / raw)
To: Git Mailing List
In sha1-file.c:read_object_file_extended() we have the following pattern:
errno = 0;
data = read_object(r, repl, type, size);
if (data)
return data;
if (errno && errno != ENOENT)
die_errno(_("failed to read object %s"), oid_to_hex(oid));
That is, it is expected that read_object() does not change the value of
errno in the non-error case. I find it intriguing that we expect a quite
large call graph that is behind read_object() to behave this way.
What if a subordinate callee starts doing
if (some_syscall(...) < 0) {
if (errno == EEXIST) {
/* never mind, that's OK */
...
}
}
Would it be required to reset errno to its previous value in this
failure-is-not-an-error case?
The problem in my Windows build is that one of these subordinate
syscalls is vsnprintf() (as part of a strbuf_add variant, I presume),
and it fails with ERANGE when the buffer is too short. Do I have to
modify the vsnprintf emulation to restore errno?
^ permalink raw reply
* Re: Updating the commit message for reverts
From: Danh Doan @ 2019-12-28 13:20 UTC (permalink / raw)
To: Gal Paikin; +Cc: Junio C Hamano, git
In-Reply-To: <CAEsQYpMJGbw3L66vCd25Ht0bTBzvvt1yMRd2U3=u3U-BZukyzg@mail.gmail.com>
On 2019-12-27 11:13:47+0100, Gal Paikin <paiking@google.com> wrote:
> Hi,
> Thanks for the reply!
>
> So the idea of changing from "Revert Revert" to "Reland", "reapply"
> has a big problem: sometimes Revert^2 actually means 'reverting
> "Revert"' since "Revert" introduced a bug that wasn't in the original
> change.
>
> So to your question, I don't know what Revert^47 means since it
> depends on each individual case. Sometimes it actually means "Revert"
> and sometimes it means "Reland".
>
> So do people actually use it? Yes! Many users reported to me that it
> is not that unusual to get to "Revert^6", and it is very usual and
I've seen Revert x6 in a code base, I couldn't get to know the reason
for that reversion war. I think it could be seen more in some in-house
web development that uses trunk-based development, code is being
tested with CI/CD, lightly tested, squash-merged to master,
then run into problem in staging (or worst, production, because not
enough traffix was generated for testing environment).
> common to get to "Revert^2/3/4". It is also useful for the users to
> know the number of the revert, according to the reports. Here is an
> example:
> https://android-review.googlesource.com/c/platform/art/+/352330
> Feel free to also search for "Revert^2/3/4" to find many results.
>
> Anyway, I am certain that "Revert^3" is better than "Revert revert
> revert". There is definitely no clear way to solve this issue, but
> perhaps "nth revert" would be a more "human language" solution?
In my very personal opinion, "nth revert" is a poor choice.
At a first glance, I would take it as:
This is the "nth revert", after applying this patch n times.
--
Danh
^ permalink raw reply
* Re: [ANNOUNCE] Git v2.25.0-rc0
From: Danh Doan @ 2019-12-28 12:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq36d5jtck.fsf@gitster-ct.c.googlers.com>
On 2019-12-27 14:28:59-0800, Junio C Hamano <gitster@pobox.com> wrote:
> Danh Doan <congdanhqx@gmail.com> writes:
> > Some of my projects requires ASCII-only user.name,
> > instead of doing the right thing
> >
> > git config user.name <simplified-name>
> >
> > I decided to set it globally instead.
> > I rarely need to type in my native language,
> > hence I don't have the IME software start with Xorg.
>
> Hmph, but back in v2.20 days, you did have IME?
Back in the days, I had my IME started on logging in,
then to save some tiny time on starting up,
I turned it off.
>
> In any case, if I were in such a situation to need my name spelled
> differently depending on the project I work on, I would probably use
>
> $ git config --global user.name <simplified-name>
> $ cd <repository of git>
> $ git config user.name <name-with-accents>
>
> or the other way around (depends on which projects your focus is on).
Yes, that's the the right way to do, but I didn't pick that way.
I'm going through my ~/src and fixing it.
Thanks,
--
Danh
^ permalink raw reply
* Re: [PATCH 04/16] t2018: teach do_checkout() to accept `!` arg
From: Eric Sunshine @ 2019-12-28 8:34 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
In-Reply-To: <4eab751a3cf00bbffaf4b1084928dad264fa1572.1577454401.git.liu.denton@gmail.com>
On Fri, Dec 27, 2019 at 8:47 AM Denton Liu <liu.denton@gmail.com> wrote:
> Before, we were running `test_must_fail do_checkout`. However,
> `test_must_fail` should only be used on git commands. Teach
> do_checkout() to accept `!` as a potential first argument which will
> prepend `test_must_fail` to the enclosed git command and skips the
> remainder of the function.
There's a grammatical problem here. s/skips/skip/ is one way to fix it.
Use imperative mood when writing commit messages. Drop words such as
"before" and "were". For instance:
Stop using test_must_fail() with non-Git commands because...
(Same comment applies to pretty much all commit messages in this series.)
> This increases the granularity of the test as, instead of blindly
> checking that do_checkout() failed, we check that only the specific
> expected invocation of git fails.
This may be a case of trying to describe in prose too much of what is
better described by the code itself. As a reviewer, I spent more time
trying to figure out what this was saying that I did merely looking at
the code and comprehending why the two checks following the
git-checkout invocation should be skipped. Consequently, I lean toward
dropping "...and skips the remainder..." through the end of the commit
message.
More below...
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
> @@ -13,6 +13,12 @@ test_description='checkout'
> #
> # If <checkout options> is not specified, "git checkout" is run with -b.
> do_checkout () {
> + should_fail= &&
> + if test "x$1" = "x!"
> + then
> + should_fail=test_must_fail &&
> + shift
> + fi &&
You forgot to update the function comment to talk about the new
optional "!" argument.
> @@ -26,10 +32,13 @@ do_checkout () {
> - git checkout $opts $exp_branch $exp_sha &&
> + $should_fail git checkout $opts $exp_branch $exp_sha &&
If I read this literally, it says that the git checkout should always
fail. A more wisely chosen variable name would help to alleviate this
problem.
When you start parameterizing the actual invocation of a command like
this (I'm not talking about the command arguments which are also
parameterized), the abstraction level and cognitive load increase...
> - test $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&
> - test $exp_sha = $(git rev-parse --verify HEAD)
> + if test -z "$should_fail"
> + then
> + test $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&
> + test $exp_sha = $(git rev-parse --verify HEAD)
> + fi
> }
You could reduce the cognitive load by making the code easier to
understand at-a-glance (though at the cost of a minor bit of
duplication) by structuring it instead like this:
if test -n "$should_fail"
then
test_must_fail git checkout $opts $exp_branch $exp_sha
else
git checkout $opts $exp_branch $exp_sha &&
test $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&
test $exp_sha = $(git rev-parse --verify HEAD)
fi
where 'should_fail' is either empty or non-empty depending upon
whether "!" was supplied as an argument. (And, when coded this way,
"should_fail" is a reasonable variable name.)
^ permalink raw reply
* Re: [PATCH 03/16] t2018: use test_must_fail for failing git commands
From: Eric Sunshine @ 2019-12-28 7:55 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
In-Reply-To: <c584c9a52b492db2128846e86afb0aadddd6f2de.1577454401.git.liu.denton@gmail.com>
On Fri, Dec 27, 2019 at 8:47 AM Denton Liu <liu.denton@gmail.com> wrote:
> We had some instances of `test_must_fail test_dirty_{un,}mergable`.
> However, `test_must_fail` should only be used on git commands. Teach
> test_dirty_{un,}mergable() to accept `!` as a potential first argument
s/mergable/mergeable/ twice above.
> which will run the git command without test_must_fail(). This prevents
> the double-negation where we were effectively running
> `test_must_fail test_must_fail git diff ...`.
This change makes the situation even more confusing than it already
is. For instance, you can now say:
test_dirty_unmergable !
which effectively says "not unmergeable", which is the same as "not
not mergeable". Does that mean it is mergeable? Does that mean it
should be calling test_dirty_mergeable()? Same situation arises with:
test_dirty_mergeable !
It seems like there are four distinct cases that are being tested
here. If that's so, then rather than changing these functions to
accept "!" as an argument, perhaps there should be four distinct
one-liner functions for testing those cases?
^ permalink raw reply
* Re: [PATCH 08/16] t3310: extract common no_notes_merge_left()
From: Eric Sunshine @ 2019-12-28 7:20 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
In-Reply-To: <68eb05e10edf1f4b09bc4e02587ac5b15fd2c6e3.1577454401.git.liu.denton@gmail.com>
On Fri, Dec 27, 2019 at 8:48 AM Denton Liu <liu.denton@gmail.com> wrote:
> diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
> @@ -32,6 +32,11 @@ verify_notes () {
> +no_notes_merge_left () {
> + { ls .git/NOTES_MERGE_* >output || :; } &&
> + test_must_be_empty output
> +}
This function name leaves me thinking that it's talking about
directionality (left vs. right) and gives insufficient clue that it's
talking about a .git/NOTES_MERGE_* file. A name such as
assert_no_notes_merge_files() or notes_merge_files_gone() would make
the intention more obvious.
> - # No .git/NOTES_MERGE_* files left
> - test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
> - test_must_be_empty output &&
On the other hand, the original in-code comment was not confusing,
probably because it was obvious it was talking about an actual file,
due to spelling out .git/NOTES_MERGE_* explicitly and due to actually
using the literal word "file", plus the code following the comment
made it very obvious what was happening.
These observations may not be actionable since someone actually
working on this script will know that it's dealing with
.git/NOTES_MERGES_*, but as a reviewer not familiar with this
particular script, reading the patch from top to bottom, I found the
function name confusing.
^ permalink raw reply
* Re: [PATCH] revision: allow missing promisor objects on CLI
From: Junio C Hamano @ 2019-12-28 3:50 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, matvore
In-Reply-To: <20191228003430.241283-1-jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> object = get_reference(revs, arg, &oid, flags ^ local_flags);
> if (!object)
> - return revs->ignore_missing ? 0 : -1;
> + /*
> + * Either this object is missing and ignore_missing is true, or
> + * this object is a (missing) promisor object and
> + * exclude_promisor_objects is true.
I had to guess and dig where these assertions are coming from; we
should not force future readers of the code to.
At least this comment must say why these assertions hold. Say
something like "get_reference() yields NULL on only such and such
cases" before concluding with "and in any of these cases, we can
safely ignore it because ...".
I think the two cases the comment covers are safe for this caller to
silently return 0. Another case get_reference() yields NULL is when
oid_object_info() says it is a commit but it turns out that the
object is found by repo_parse_commit() to be a non-commit, isn't it?
I am not sure if it is safe for this caller to just return 0. There
may be some other "unusual-but-not-fatal" cases where get_reference()
does not hit a die() but returns NULL.
Thanks.
^ permalink raw reply
* [PATCH] revision: allow missing promisor objects on CLI
From: Jonathan Tan @ 2019-12-28 0:34 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, matvore
Commit 4cf67869b2 ("list-objects.c: don't segfault for missing cmdline
objects", 2018-12-06) prevented some segmentation faults from occurring
by tightening handling of missing objects provided through the CLI: if
--ignore-missing is set, then it is OK (and the missing object ignored,
just like one would if encountered in traversal).
However, in the case that --ignore-missing is not set but
--exclude-promisor-objects is set, there is still no distinction between
the case wherein the missing object is a promisor object and the case
wherein it is not. This is unnecessarily restrictive, since if a missing
promisor object is encountered in traversal, it is ignored; likewise it
should be ignored if provided through the CLI. Therefore, distinguish
between these 2 cases. (As a bonus, the code is now simpler.)
(Note that this only affects handling of missing promisor objects.
Handling of non-missing promisor objects is already done by setting all
of them to UNINTERESTING in prepare_revision_walk().)
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
For those curious, I discovered this while trying to extend the
check-targets-only optimization done in dfa33a298d ("clone: do faster
object check for partial clones", 2019-04-21) to fetches as well. While
investigating my previous work of adding check-target functionality in
addition to the usual connectivity check (for correctness, not for
performance) in 35f9e3e5e7 ("fetch: in partial clone, check presence of
targets", 2018-09-21), I discovered that at current master, the check
was somehow no longer needed because rev-list dies on missing objects on
CLI. But I don't think that the current behavior is obvious, hence this
commit (which also restores the need for the check-target
functionality).
---
revision.c | 10 +++++++++-
t/t0410-partial-clone.sh | 10 ++--------
2 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/revision.c b/revision.c
index 8136929e23..345615e300 100644
--- a/revision.c
+++ b/revision.c
@@ -1907,7 +1907,15 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
verify_non_filename(revs->prefix, arg);
object = get_reference(revs, arg, &oid, flags ^ local_flags);
if (!object)
- return revs->ignore_missing ? 0 : -1;
+ /*
+ * Either this object is missing and ignore_missing is true, or
+ * this object is a (missing) promisor object and
+ * exclude_promisor_objects is true. In any case, we are
+ * allowed to skip processing of this object; this object will
+ * not appear in output and cannot be used as a source of
+ * UNINTERESTING ancestors (since it is missing).
+ */
+ return 0;
add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);
add_pending_object_with_path(revs, object, arg, oc.mode, oc.path);
free(oc.path);
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index a3988bd4b8..fd28f5402a 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -416,15 +416,9 @@ test_expect_success 'rev-list dies for missing objects on cmd line' '
git -C repo config extensions.partialclone "arbitrary string" &&
for OBJ in "$COMMIT" "$TREE" "$BLOB"; do
- test_must_fail git -C repo rev-list --objects \
+ git -C repo rev-list --objects \
--exclude-promisor-objects "$OBJ" &&
- test_must_fail git -C repo rev-list --objects-edge-aggressive \
- --exclude-promisor-objects "$OBJ" &&
-
- # Do not die or crash when --ignore-missing is passed.
- git -C repo rev-list --ignore-missing --objects \
- --exclude-promisor-objects "$OBJ" &&
- git -C repo rev-list --ignore-missing --objects-edge-aggressive \
+ git -C repo rev-list --objects-edge-aggressive \
--exclude-promisor-objects "$OBJ"
done
'
--
2.24.1.735.g03f4e72817-goog
^ permalink raw reply related
* Re: [PATCH 12/16] t3504: don't use `test_must_fail test_cmp`
From: Junio C Hamano @ 2019-12-27 22:48 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Denton Liu, Git Mailing List
In-Reply-To: <41175b96-37d7-f550-b9c2-41d569145b91@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> This is VERY suspicious. It is not reliable to check that something
> is not exactly equal to something else.
>
> Feel free to replace this patch by the following.
>
> ----- 8< -----
> t3504: do check for conflict marker after failed cherry-pick
>
> The test with disabled rerere should make sure that the cherry-picked
> result does not have the conflict replaced with a recorded resolution.
>
> It attempts to do so by ensuring that the file content is _not_ equal
> to some other file. That by itself is a very dubious check because just
> about every random result of an incomplete cherry-pick would satisfy
> the condition.
Good thinking. Thanks for catching the bogosity of the original
test, which dates back to more than 10 years ago X-<.
> In this case, the intent was to check that the conflicting file does
> _not_ contain the resolved content. But the check actually uses the
> wrong reference file, but not the resolved content. Needless to say
> that the non-equality is satisfied. And, on top of it, it uses a commit
> that does not even touch the file that is checked.
>
> Do check for the expected result, which is content from both sides of
> the merge and merge conflicts. (The latter we check for just the
> middle separator for brevity.)
>
> As a side-effect, this also removes an incorrect use of test_must_fail.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> t/t3504-cherry-pick-rerere.sh | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh
> index a267b2d144..80a0d08706 100755
> --- a/t/t3504-cherry-pick-rerere.sh
> +++ b/t/t3504-cherry-pick-rerere.sh
> @@ -94,8 +94,10 @@ test_expect_success 'cherry-pick --rerere-autoupdate more than once' '
>
> test_expect_success 'cherry-pick conflict without rerere' '
> test_config rerere.enabled false &&
> - test_must_fail git cherry-pick master &&
> - test_must_fail test_cmp expect foo
> + test_must_fail git cherry-pick foo-master &&
> + grep ===== foo &&
> + grep foo-dev foo &&
> + grep foo-master foo
> '
>
> test_done
^ permalink raw reply
* Re: Comparing rebase --am with --interactive via p3400
From: Elijah Newren @ 2019-12-27 22:45 UTC (permalink / raw)
To: Alban Gruin; +Cc: Johannes Schindelin, Git Mailing List
In-Reply-To: <a00e1689-ec7c-4039-a2e9-f72d452ae4ff@gmail.com>
Hi Alban,
On Fri, Dec 27, 2019 at 1:11 PM Alban Gruin <alban.gruin@gmail.com> wrote:
>
> Hi Johannes & Elijah,
>
> Le 01/02/2019 à 07:04, Johannes Schindelin a écrit :
> > Hi Elijah,
> >
> > as discussed at the Contributors' Summit, I ran p3400 as-is (i.e. with the
> > --am backend) and then with --keep-empty to force the interactive backend
> > to be used. Here are the best of 10, on my relatively powerful Windows 10
> > laptop, with current `master`.
> >
> > With regular rebase --am:
> >
> > 3400.2: rebase on top of a lot of unrelated changes 5.32(0.06+0.15)
> > 3400.4: rebase a lot of unrelated changes without split-index 33.08(0.04+0.18)
> > 3400.6: rebase a lot of unrelated changes with split-index 30.29(0.03+0.18)
> >
> > with --keep-empty to force the interactive backend:
> >
> > 3400.2: rebase on top of a lot of unrelated changes 3.92(0.03+0.18)
> > 3400.4: rebase a lot of unrelated changes without split-index 33.92(0.03+0.22)
> > 3400.6: rebase a lot of unrelated changes with split-index 38.82(0.03+0.16)
> >
> > I then changed it to -m to test the current scripted version, trying to
> > let it run overnight, but my laptop eventually went to sleep and the tests
> > were not even done. I'll let them continue and report back.
> >
> > My conclusion after seeing these numbers is: the interactive rebase is
> > really close to the performance of the --am backend. So to me, it makes a
> > total lot of sense to switch --merge over to it, and to make --merge the
> > default. We still should investigate why the split-index performance is so
> > significantly worse, though.
> >
> > Ciao,
> > Dscho
> >
>
> I investigated a bit on this. From a quick glance at a callgrind trace,
> I can see that ce_write_entry() is called 20 601[1] times with `git am',
> but 739 802 times with the sequencer when the split-index is enabled.
Sweet, thanks for digging in and analyzing this.
> For reference, here are the timings, measured on my Linux machine, on a
> tmpfs, with git.git as the repo:
>
> `rebase --am':
> > 3400.2: rebase on top of a lot of unrelated changes 0.29(0.24+0.03)
> > 3400.4: rebase a lot of unrelated changes without split-index 6.77(6.51+0.22)
> > 3400.6: rebase a lot of unrelated changes with split-index 4.43(4.29+0.13)
> `rebase --quiet':
--quiet? Isn't that flag supposed to work with both backends and not
imply either one? We previously used --keep-empty, though there's a
chance that flag means we're not doing a fair comparison (since 'am'
will drop empty commits and thus have less work to do). Is there any
chance you actually ran a different command, but when you went to
summarize just typed the wrong flag name? Anyway, the best would
probably be to use --merge here (at the time Johannes and I were
testing, that wouldn't have triggered the sequencer, but it does now),
after first applying the en/rebase-backend series just to make sure
we're doing an apples to apples comparison. However, I suspect that
empty commits probably weren't much of a factor and you did find some
interesting things...
> > 3400.2: rebase on top of a lot of unrelated changes 0.24(0.21+0.02)
> > 3400.4: rebase a lot of unrelated changes without split-index 5.60(5.32+0.27)
> > 3400.6: rebase a lot of unrelated changes with split-index 5.67(5.40+0.26)
>
> This comes from two things:
>
> 1. There is not enough shared entries in the index with the sequencer.
>
> do_write_index() is called only by do_write_locked_index() with `--am',
> but is also called by write_shared_index() with the sequencer once for
> every other commit. As the latter is only called by
> write_locked_index(), which means that too_many_not_shared_entries()
> returns true for the sequencer, but never for `--am'.
>
> Removing the call to discard_index() in do_pick_commit() (as in the
> first attached patch) solve this particular issue, but this would
> require a more thorough analysis to see if it is actually safe to do.
I'm actually surprised the sequencer would call discard_index(); I
would have thought it would have relied on merge_recursive() to do the
necessary index changes and updates other than writing the new index
out. But I'm not quite as familar with the sequencer so perhaps
there's some reason I'm unaware of. (Any chance this is a left-over
from when sequencer invoked external scripts to do the work, and thus
the index was updated in another processes' memory and on disk, and it
had to discard and re-read to get its own process updated?)
> After this, ce_write() is still called much more by the sequencer.
>
> Here are the results of `rebase --quiet' without discarding the index:
>
> > 3400.2: rebase on top of a lot of unrelated changes 0.23(0.19+0.04)
> > 3400.4: rebase a lot of unrelated changes without split-index 5.14(4.95+0.18)
> > 3400.6: rebase a lot of unrelated changes with split-index 5.02(4.87+0.15)
> The performance of the rebase is better in the two cases.
Nice. :-)
> 2. The base index is dropped by unpack_trees_start() and unpack_trees().
>
> Now, write_shared_index() is no longer called and write_locked_index()
> is less expensive than before according to callgrind. But
> ce_write_entry() is still called 749 302 times (which is even more than
> before.)
>
> The only place where ce_write_entry() is called is in a loop in
> do_write_index(). The number of iterations is dictated by the size of
> the cache, and there is a trace2 probe dumping this value.
>
> For `--am', the value goes like this: 1, 2, 2, 2, 2, 3, 3, 3, 3, 4, 4,
> 4, 4, 5, 5, 5, 5, … up until 101.
>
> For the sequencer, it goes like this: 1, 1, 3697, 3697, 3698, 3698,
> 3699, 3699, … up until 3796.
>
> The size of the cache is set in prepare_to_write_split_index(). It
> grows if a cache entry has no index (most of them should have one by
> now), or if the split index has no base index (with `--am', the split
> index always has a base.) This comes from unpack_trees_start() -- it
> creates a new index, and unpack_trees() does not carry the base index,
> hence the size of the cache.
>
> The second attached patch (which is broken for the non-interactive
> rebase case) demonstrates what we could expect for the split-index case
> if we fix this:
>
> > 3400.2: rebase on top of a lot of unrelated changes 0.24(0.21+0.03)
> > 3400.4: rebase a lot of unrelated changes without split-index 5.81(5.62+0.17)
> > 3400.6: rebase a lot of unrelated changes with split-index 4.76(4.54+0.20)
> So, for everything related to the index, I think that’s it.
>
> [1] Numbers may vary, but they should remain in the same order of magnitude.
Unfortunately, this patch as-is breaks some important things even if
it only shows up in a few testcases. merge-recursive needs to know
both what the index looked like before the merge started, as well as
what it looks like after unpack-trees runs; see commits 1de70dbd1a
(merge-recursive: fix check for skipability of working tree updates,
2018-04-19) and a35edc84bd (merge-recursive: fix was_tracked() to quit
lying with some renamed paths, 2018-04-19), and maybe a few others
from that series.
But, noting that it comes from the differences in the index as
unpack_trees runs is useful info. I might be restructuring this code
somewhat significantly but it helps to have this in mind; I may spot
opportunities to do something with it while I'm digging in...
Elijah
^ permalink raw reply
* Re: [PATCH v2 2/2] sparse-checkout: document interactions with submodules
From: Junio C Hamano @ 2019-12-27 22:33 UTC (permalink / raw)
To: Elijah Newren
Cc: Derrick Stolee via GitGitGadget, Git Mailing List,
SZEDER Gábor, jon, Derrick Stolee
In-Reply-To: <CABPp-BHnTyYJ=RgQhnrRvcwSRd=kGHR=j5uvuVRYZjDNYAdX8Q@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> On Fri, Dec 27, 2019 at 10:47 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> Junio asked what the behavior is between the sparse-checkout feature
>> and the submodule feature.
>
> Does this first sentence help future readers? It is what spurred you
> to write the documentation, but it seems like something that could
> just be left out.
I tend to agree. It can safely go below the three-dash lines.
^ permalink raw reply
* Re: [PATCH 1/1] git-gui: add possibility to open currently selected file
From: Junio C Hamano @ 2019-12-27 22:32 UTC (permalink / raw)
To: Pratyush Yadav; +Cc: Zoli Szabó via GitGitGadget, git, Zoli Szabó
In-Reply-To: <20191227193418.36uzeizs37nv7ywb@yadavpratyush.com>
Pratyush Yadav <me@yadavpratyush.com> writes:
> Have you tested it on Windows? Does 'explorer.exe' do the correct thing?
>
> Looking at MacOS's 'open' man page, I think it should also work like
> xdg-open and shouldn't be a problem.
> ...
> Tested on Linux. Works fine. Looking forward to the re-roll.
> Subject: Re: [PATCH 1/1] git-gui: add possibility to open currently selected file
The phrasing on the title is a bit awkward. "add possibility to do
X" is better spelled "allow doing X", no?
^ 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