* [PATCH] reset: --unmerge
From: Junio C Hamano @ 2016-10-24 23:10 UTC (permalink / raw)
To: git
The procedure to resolve a merge conflict typically goes like this:
- first open the file in the editor, and with the help of conflict
markers come up with a resolution.
- save the file.
- look at the output from "git diff" to see the combined diff to
double check if the resolution makes sense.
- perform other tests, like trying to build the result with "make".
- finally "git add file" to mark that you are done.
and repeating the above until you are done with all the conflicted
paths. If you, for whatever reason, accidentally "git add file" by
mistake until you are convinced that you resolved it correctly (e.g.
doing "git add file" immediately after saving, without a chance to
peruse the output from "git diff"), there is no good way to recover.
There is "git checkout -m file" but that overwrites the working tree
file to reproduce the conflicted state, which is not exactly what
you want. You only want to reproduce the conflicted state in the
index, so that you can inspect the (proposed) merge resolution you
already have in your working tree.
Add "git reset --unmerge <paths>" command that does just that.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* Not thought through thoroughly yet about details, such as "should
it always require at least one pathspec?".
builtin/reset.c | 8 +++++++-
t/t7107-reset-unmerged.sh | 36 ++++++++++++++++++++++++++++++++++++
2 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 9020ec66c8..3aa9e0b34a 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -21,6 +21,7 @@
#include "parse-options.h"
#include "unpack-trees.h"
#include "cache-tree.h"
+#include "resolve-undo.h"
static const char * const git_reset_usage[] = {
N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]"),
@@ -272,6 +273,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
struct object_id oid;
struct pathspec pathspec;
int intent_to_add = 0;
+ int unmerge = 0;
const struct option options[] = {
OPT__QUIET(&quiet, N_("be quiet, only report errors")),
OPT_SET_INT(0, "mixed", &reset_type,
@@ -286,6 +288,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
OPT_BOOL('p', "patch", &patch_mode, N_("select hunks interactively")),
OPT_BOOL('N', "intent-to-add", &intent_to_add,
N_("record only the fact that removed paths will be added later")),
+ OPT_BOOL(0, "unmerge", &unmerge,
+ N_("recover conflicted stages from an earlier 'git add'")),
OPT_END()
};
@@ -357,7 +361,9 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
hold_locked_index(lock, 1);
if (reset_type == MIXED) {
int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;
- if (read_from_tree(&pathspec, oid.hash, intent_to_add))
+ if (unmerge)
+ unmerge_cache(&pathspec);
+ else if (read_from_tree(&pathspec, oid.hash, intent_to_add))
return 1;
if (get_git_work_tree())
refresh_index(&the_index, flags, NULL, NULL,
diff --git a/t/t7107-reset-unmerged.sh b/t/t7107-reset-unmerged.sh
new file mode 100755
index 0000000000..57b2e27150
--- /dev/null
+++ b/t/t7107-reset-unmerged.sh
@@ -0,0 +1,36 @@
+#!/bin/sh
+
+test_description='git reset with paths'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+ echo one >file &&
+ git add file &&
+ git commit -m "one" &&
+ git tag initial &&
+
+ echo two >file &&
+ git commit -a -m "two" &&
+
+ git checkout -b side initial &&
+ echo three >file &&
+ git commit -a -m "three"
+'
+
+test_expect_success "cause conflict, resolve, and unresolve" '
+ git reset --hard &&
+ git checkout master &&
+ test_must_fail git merge side &&
+
+ git ls-files -u >expect &&
+
+ echo four >file &&
+ git add file &&
+
+ git reset --unmerge -- file &&
+ git ls-files -u >actual &&
+ test_cmp expect actual
+'
+
+test_done
^ permalink raw reply related
* Re: An anomaly, not a bug
From: Alexei Lozovsky @ 2016-10-24 22:47 UTC (permalink / raw)
To: m.; +Cc: git
In-Reply-To: <CALhvvbYnTf9qqsCFWjgfnynRy9LDEQf=Y++2tQioTKHbaYGHDA@mail.gmail.com>
> Actually, git reads
>
> # comment
>
> as 'ignore files with name " " (4 spaces)', and then a comment.
> It does not ignore the leading whitespace.
Even not as a comment, it treats it as literally a filename with
a hash and that comment in it. However, one (usually) does not
name their files like that, so for all purposes this can be thought
of as a sort of comment.
^ permalink raw reply
* Re: [PATCH] hex: use unsigned index for ring buffer
From: René Scharfe @ 2016-10-24 22:33 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: Git List
In-Reply-To: <xmqqshrl7j42.fsf@gitster.mtv.corp.google.com>
Am 24.10.2016 um 19:27 schrieb Junio C Hamano:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> I think it would be preferable to just fix it inline in each place.
>>
>> Yeah, probably.
>>
>> My initial reaction to this was
>>
>> char *sha1_to_hex(const unsigned char *sha1)
>> {
>> - static int bufno;
>> + static unsigned int bufno;
>> static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
>> return sha1_to_hex_r(hexbuffer[3 & ++bufno], sha1);
>>
>> "ah, we do not even need bufno as uint; it could be ushort or even
>> uchar". If this were a 256 element ring buffer and the index were
>> uchar, we wouldn't even be having this discussion, and "3 &" is a
>> way to get a fake type that is a 2-bit unsigned integer that wraps
>> around when incremented.
>>
>> But being explicit, especially when we know that we can rely on the
>> fact that the compilers are usually intelligent enough, is a good
>> idea, I would think.
>>
>> Isn't size_t often wider than uint, by the way? It somehow makes me
>> feel dirty to use it when we know we only care about the bottom two
>> bit, especially with the explicit "bufno %= ARRAY_SIZE(hexbuffer)",
>> but I may be simply superstitious in this case. I dunno.
>
> If we are doing the wrap-around ourselves, I suspect that the index
> should stay "int" (not even unsigned), as that is supposed to be the
> most natural and performant type on the architecture. Would it
> still result in better code to use size_t instead?
Right.
Gcc emits an extra cltq instruction for x86-64 (Convert Long To Quad,
i.e. 32-bit to 64-bit) if we wrap explicitly and still use an int in
sha1_to_hex(). It doesn't if we use an unsigned int or size_t. It
also doesn't for get_pathname(). I guess updating the index variable
only after use makes the difference there.
But I think we can ignore that; it's just an extra cycle. I only
even noticed it when verifying that "% 4" is turned into "& 3"..
Clang and icc don't add the cltq, by the way.
So how about this? It gets rid of magic number 3 and works for array
size that's not a power of two. And as a nice side effect it can't
trigger a signed overflow anymore.
Just adding "unsigned" looks more attractive to me for some reason.
Perhaps I stared enough at the code to get used to the magic values
there..
René
---
hex.c | 3 ++-
path.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/hex.c b/hex.c
index ab2610e..845b01a 100644
--- a/hex.c
+++ b/hex.c
@@ -78,7 +78,8 @@ char *sha1_to_hex(const unsigned char *sha1)
{
static int bufno;
static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
- return sha1_to_hex_r(hexbuffer[3 & ++bufno], sha1);
+ bufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);
+ return sha1_to_hex_r(hexbuffer[bufno], sha1);
}
char *oid_to_hex(const struct object_id *oid)
diff --git a/path.c b/path.c
index a8e7295..52d889c 100644
--- a/path.c
+++ b/path.c
@@ -25,7 +25,8 @@ static struct strbuf *get_pathname(void)
STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
};
static int index;
- struct strbuf *sb = &pathname_array[3 & ++index];
+ struct strbuf *sb = &pathname_array[index];
+ index = (index + 1) % ARRAY_SIZE(pathname_array);
strbuf_reset(sb);
return sb;
}
--
2.10.1
^ permalink raw reply related
* Re: An anomaly, not a bug
From: Alexei Lozovsky @ 2016-10-24 22:20 UTC (permalink / raw)
To: m.; +Cc: git
In-Reply-To: <20161024214601.GA11699@hostname_variable_in_muttrc>
> Note: the comments can be started away from the left margin,
> as normal in all unix-linux configuration files we know of.
> Git follows this behaviour fine.
Actually, git reads
# comment
as 'ignore files with name " " (4 spaces)', and then a comment.
It does not ignore the leading whitespace.
^ permalink raw reply
* [PATCH] doc: fix the 'revert a faulty merge' ASCII art tab spacing
From: Philip Oakley @ 2016-10-24 21:54 UTC (permalink / raw)
To: gitster; +Cc: Johannes.Schindelin, git, peff, philipoakley
In-Reply-To: <xmqq1sz9b9ex.fsf@gitster.mtv.corp.google.com>
The asciidoctor doc-tool stack does not always respect the 'tab = 8 spaces' rule
expectation, particularly for the Git-for-Windows generated html pages. This
follows on from the 'doc: fix merge-base ASCII art tab spacing' fix.
Use just spaces within the block of the ascii art.
All other *.txt ascii art containing three dashes has been checked.
Asciidoctor correctly formats the other art blocks that do contain tabs.
Signed-off-by: Philip Oakley <philipoakley@iee.org
---
The git-scm doc pages https://git-scm.com/docs/ does not convert this
how-to document to html, rather it links to the Github text pages, which
does respect the 8 space tab rule.
---
Documentation/howto/revert-a-faulty-merge.txt | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/Documentation/howto/revert-a-faulty-merge.txt b/Documentation/howto/revert-a-faulty-merge.txt
index 462255e..19f59cc 100644
--- a/Documentation/howto/revert-a-faulty-merge.txt
+++ b/Documentation/howto/revert-a-faulty-merge.txt
@@ -30,7 +30,7 @@ The history immediately after the "revert of the merge" would look like
this:
---o---o---o---M---x---x---W
- /
+ /
---A---B
where A and B are on the side development that was not so good, M is the
@@ -47,7 +47,7 @@ After the developers of the side branch fix their mistakes, the history
may look like this:
---o---o---o---M---x---x---W---x
- /
+ /
---A---B-------------------C---D
where C and D are to fix what was broken in A and B, and you may already
@@ -81,7 +81,7 @@ In such a situation, you would want to first revert the previous revert,
which would make the history look like this:
---o---o---o---M---x---x---W---x---Y
- /
+ /
---A---B-------------------C---D
where Y is the revert of W. Such a "revert of the revert" can be done
@@ -93,14 +93,14 @@ This history would (ignoring possible conflicts between what W and W..Y
changed) be equivalent to not having W or Y at all in the history:
---o---o---o---M---x---x-------x----
- /
+ /
---A---B-------------------C---D
and merging the side branch again will not have conflict arising from an
earlier revert and revert of the revert.
---o---o---o---M---x---x-------x-------*
- / /
+ / /
---A---B-------------------C---D
Of course the changes made in C and D still can conflict with what was
@@ -111,13 +111,13 @@ faulty A and B, and redone the changes on top of the updated mainline
after the revert, the history would have looked like this:
---o---o---o---M---x---x---W---x---x
- / \
+ / \
---A---B A'--B'--C'
If you reverted the revert in such a case as in the previous example:
---o---o---o---M---x---x---W---x---x---Y---*
- / \ /
+ / \ /
---A---B A'--B'--C'
where Y is the revert of W, A' and B' are rerolled A and B, and there may
@@ -129,7 +129,7 @@ lot of overlapping changes that result in conflicts. So do not do "revert
of revert" blindly without thinking..
---o---o---o---M---x---x---W---x---x
- / \
+ / \
---A---B A'--B'--C'
In the history with rebased side branch, W (and M) are behind the merge
--
2.9.0.windows.1.323.g0305acf
^ permalink raw reply related
* An anomaly, not a bug
From: m. @ 2016-10-24 21:46 UTC (permalink / raw)
To: git
Concerning ".gitignore", experienced using git 2.10.0
Starting code using one or more spaces or tabs from the left
margin will have git reading .gitignore and ignoring(or
un-ignoring) the command-instruction.
Example: Starting .gitignore
/*
# above line is duly read. Then un-ignoring
# something but starting the command further to the
# right will have git not reading that line
!/nottobeignored.file
End .gitignore
Note: the comments can be started away from the left margin,
as normal in all unix-linux configuration files we know of.
Git follows this behaviour fine.
The lines containing commands, on the contrary of regular
convention cannot be indented by spaces or tabs. Quite
unusual, confusing, not in the sense of conventional and
easily adverted in coding git.
Could be we are missing out on something,
Git command line tool is a functional tool now in our setup
for two years, first noticed this alien behaviour in this
version of git, on osx, the fink(osx package mananger)
binary.
^ permalink raw reply
* Re: revisiting zstd timings
From: Junio C Hamano @ 2016-10-24 21:36 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161023080552.lma2v6zxmyaiiqz5@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> If we were designing git today, it seems like a no-brainer to use zstd
> over zlib. But given backwards-compatibility issues, I'm not sure.
> 10-20% speedup on reading is awfully nice, but I don't think there's a
> good way to gracefully transition, because zlib is part of the
> on-the-wire format for serving objects. We could re-compress on the fly,
> but that gets expensive (in existing cases, we can quite often serve the
> zlib content straight from disk, but this would require an extra
> inflate/deflate. At least we wouldn't have to reconstitute objects from
> deltas, though).
>
> A transition would probably look something like:
>
> 0. The patch below, or something like it, to teach git to read both
> zlib and zstd, and optionally write zstd. We'd probably want to
> make this an unconditional requirement like zlib, because the point
> is for it to be available everywhere (I assume the zstd code is
> pretty portable, but I haven't put it to the test).
>
> 1. Another patch to add a "zstd" capability to the protocol. This
> would require teaching pack-objects an option to convert zstd back
> to zlib on the fly.
>
> Servers which handle a limited number of updated clients can switch
> to zstd immediately to get the benefit, and their clients can
> handle it directly. Likewise, clients of older servers may wish to
> repack locally using zstd to get the benefit. They'll have to
> recompress on the fly during push, but pushes are rare than other
> operations (and often limited by bandwidth anyway).
>
> 2. After a while, eventually flip the default to zstd=5.
>
> 3. If "a while" is long enough, perhaps add a patch to let servers
> tell clients "go upgrade" rather than recompressing on the fly.
>
> I don't have immediate plans for any of that, but maybe something to
> think about.
Thanks for a write-up. This is quite interesting.
Thanks to d98b46f8d, this does not have to impact the object naming
;-)
^ permalink raw reply
* Re: Reporting Bug in Git Version Control System
From: Junio C Hamano @ 2016-10-24 21:14 UTC (permalink / raw)
To: Stefan Beller; +Cc: Yash Jain, git@vger.kernel.org
In-Reply-To: <xmqqzilt61jo.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Mon, Oct 24, 2016 at 7:28 AM, Yash Jain <yashjain.lnm@gmail.com> wrote:
>>> Hello,
>>> I have two accounts on github("yj291197" and "yaki29").
>>> Both the accounts have different gmail IDs("yj291197@gmail.com" and
>>> "yashjain.lnm@gmail.com" respectively) but same passwords.
>>> I used to use git for "yj291197" account and a few days earlier I made
>>> this new account and used git commit to commit on "yaki29" but it
>>> appeared as "yj291197" committed on "yaki29's" repo.
>>> Then I pulled a request of that commit then it appeared "yaki29"
>>> pulled a request with a commit of "yj291197".
>>>
>>> And during this whole session I was signed in as "yaki29" on github.com .
>>>
>>
>> This is a Github issue, so ask Github support.
>>
>> Or read up on .mailmap files.
>
> I am (obviously) not a GitHub support, but I think the confusion is
> coming from not understanding who the committer and the author of a
> commit are and where they are coming from. They are both recorded
> locally, taken from user.name and user.email configuration variables
> when the commits are made. "git push" to propagate them to GitHub
> will NOT change these values of a commit, once a commit is created.
>
> The story described looks quite consistent if the user has
> yj291197@gmail.com configured as user.email and kept making commits
> in the local repository, and pushed them to either yj291197 or yaki29
> accounts at GitHub, without ever changing the local configuration to
> use the other e-mail address. All commits would record the user and
> e-mail address yj291197, and the only one that may be attributed to
> the new one yaki29 would be the automerge created at GitHub when a
> pull request is responded on-site without first fetching and making
> a merge locally.
IOW, this sounds like Pebkac to me. There is no a thing that needs
fixing in Git, and I do not immediately see there is anything GitHub
needs to fix, either. The user may need fixing, though ;-).
^ permalink raw reply
* Re: [PATCH 17/36] attr: expose validity check for attribute names
From: Stefan Beller @ 2016-10-24 21:07 UTC (permalink / raw)
To: Ramsay Jones
Cc: Junio C Hamano, git@vger.kernel.org, Brandon Williams, Duy Nguyen
In-Reply-To: <0425fea3-3419-c265-b964-f5a309b867fa@ramsayjones.plus.com>
On Sun, Oct 23, 2016 at 8:07 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
>
> On 23/10/16 00:32, Stefan Beller wrote:
>> From: Junio C Hamano <gitster@pobox.com>
>>
>> Export attr_name_valid() function, and a helper function that
>> returns the message to be given when a given <name, len> pair
>> is not a good name for an attribute.
>>
>> We could later update the message to exactly spell out what the
>> rules for a good attribute name are, etc.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>
> [snip]
>
>> +extern int attr_name_valid(const char *name, size_t namelen);
>> +extern void invalid_attr_name_message(struct strbuf *, const char *, int);
>> +
>
> The symbol 'attr_name_valid()' is not used outside of attr.c, even
> by the end of this series. Do you expect this function to be used
> in any future series? (The export is deliberate and it certainly
> seems like it should be part of the public interface, but ...)
>
> In contrast, the 'invalid_attr_name_message()' function is called
> from code in pathspec.c, which relies on 'git_attr_counted()' to
> call 'attr_name_valid()' internally to check for validity. :-D
Yeah, I am taking over Junios patches and do not quite implement
what Junio thought I would. ;) So I guess it is a communication mismatch.
git_attr_counted is a wrapper around attr_name_valid in the way that
it either returns NULL when the attr name is invalid or it does extra work
and returns a pointer to an attr.
So I think for API completeness we'd want to keep attr_name_valid around,
as otherwise the API looks strange. But that doesn't seem like a compelling
reason, so I'll drop it from the header file and make it static.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH 28/36] attr: keep attr stack for each check
From: Junio C Hamano @ 2016-10-24 20:29 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Brandon Williams, Duy Nguyen
In-Reply-To: <CAGZ79kZ_7eG4uCXwyg0F=-hjmuT1dCAATSRnY293qkNC6siM5Q@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> I looked for a platform independent way to get a thread id as a natural
> number, i.e. I want to get 1,2,3,... such that I could have just added
> list/array of attr stacks to each check, which would be the
> <check, thread> tuple you envision.
>
> However I think we do not really need it to be per check. If we had
> an easy portable way of getting such a thread id, I would have implemented
> a list of stacks per thread first. (Because each thread only looks at one
> check at a time.)
It seems that by "list of stacks per thread", you mean "there is a
list of stacks, each thread uses one and only element of that list",
but I do not think it would be desirable.
"Each thread only looks at one check at a time" is false. For
example, "write_archive_entry()" would use one check that is
specific to "git archive" to learn about "export-ignore" and
"export-subst" attributes, while letting convert_to_write_tree()
called via sha1_file_to_archive() called via write_entry() method
(i.e. write_tar_entry() or write_zip_entry()) to use a separate
check that is specific to the convert.c API.
>> With manipulation of attr stack protected with a single Big
>> Attributes Lock, this should be safe. It may not perform very well
>> when used by multiple threads, though ;-)
>
> I agree. So maybe it is not really a good fit for general consumption yet.
I would still think it is a good first step. It may already be
thread-safe, but may not be thread-ready from performance point of
view. IOW, this would not yet help an attempt to make the callers
faster by making them multi-threaded.
^ permalink raw reply
* Re: [PATCH v5 00/27] Prepare the sequencer for the upcoming rebase -i patches
From: Junio C Hamano @ 2016-10-24 20:16 UTC (permalink / raw)
To: Stefan Beller
Cc: Johannes Schindelin, git@vger.kernel.org, Jeff King,
Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <CAGZ79kaq85c1Gk1aRSrdQGp1Nm9p6tN0jXbFvTN0v+9ehooxYg@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> Speaking of what to and not to include in the upcoming release, we
>> do want to include Stefan's off-by-one fix to the submodule-helper,
>> but that is blocked on Windows end due to the test.
>
> I'd be happy either way, i.e. we could revert that fix and make a release?
> AFAICT, Windows only has broken tests, not broken functionality with that
> submodule bug fix.
If you are referring the "trailing /. should not make difference
when resolving ../relative/path" change with "rever that fix", I
think that may be a reasonable way to proceed. Even though that
change is a bugfix (at least from the point of view by me and j6t in
the recent discussion), it is a behaviour change that we would want
to see feedback from existing submodule users and deserves a longer
gestation period. And that part is not yet in 'next' yet ;-)
> If we want a longer gestation period, we'd ideally merge it to master
> just after a release, such that we "cook" it in master without having
> it in any release (we had a similar discussion for the diff heuristics IIRC).
Yes.
It would mean that we would need a separate patch that adds the
!MINGW prerequisite to some tests to what is on 'next', as the early
patches on sb/submodule-ignore-trailing-slash~ that fixes off-by-one
is the right thing to do either way. It of course needs help from
Windows folks to validate the results.
^ permalink raw reply
* Re: [PATCH v5 00/27] Prepare the sequencer for the upcoming rebase -i patches
From: Junio C Hamano @ 2016-10-24 20:03 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Stefan Beller, Jeff King, Jakub Narębski, Johannes Sixt,
Ramsay Jones
In-Reply-To: <alpine.DEB.2.20.1610231151140.3264@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> I prefer to cook it in 'next' sufficiently long to ensure that we hear
>> feedbacks from non-Windows users if there is any unexpected breakage.
>
> FWIW I am using the same patches not only on Windows but also in my Linux
> VM.
Thanks for a datapoint, but when I said "non-Windows users", I was
not referring you as "the" Windows user. I am expecting that you
would hear from Windows users who got exposure to your series by its
inclusion in Git for Windows. They are the ones that I had in mind
as "Windows users"---and not hearing breakage reported by them would
be a good sign.
The primary reason why we want to cook a new topic in 'next' is to
expose it to people with different workflows using it on different
things, and that is especially more important for a change that
affects features that are flexible and can be used in different
ways---the set of options and commands used by the original author
of the series are often different from other people's.
Any change, when it hits 'next', is expected to be sufficiently
tested by the original author [*1*], but that is only true in the
context of the original author's daily use. Both reviews and
author's tests are not sufficient to find bugs [*2*].
Topics that touch parts of the system that are more important to
users' daily Git life deserve extra time to find any unexpected
breakage in them. Windows users are participating in that test by
inclusion of the topic in the released version of Git for Windows.
I want to see the the test for the rest of the world done by early
adopters who run 'next' (as 'pu' is too scary for daily use).
[Footnote]
*1* And me, as topics geting ready to be in 'next' are first merged
to my private edition branch that is slightly ahead of 'next' to
be used in my everyday use, but just like the original author is
merely one user, I am also merely one user with a specific set
of workflows that is different from others'.
*2* Bug finding is not the primary purpose of the review in the
first place. It is to find design mistakes both at the external
and internal level, and bug finding "here you have off-by-one"
is merely a side effect. End user tests may expose the former
(e.g. the design based on a wrong assumption may not accomodate
certain workflow the original author and the reviewers failed to
consider while writing and reviewing), but no amount of test
will uncover the latter (e.g. internal API that is misdesigned
will make future enhancement unnecessarily harder).
I think it was one of the achievements of the review cycle of
this particular series that we got rid of the _entrust() thing,
for example. That had no visible external effect that would
have been caught by cooking on 'next' or releasing it to the
public, but was the kind of thing the code review was expected
to find and fix.
^ permalink raw reply
* Re: [PATCH v5 00/27] Prepare the sequencer for the upcoming rebase -i patches
From: Junio C Hamano @ 2016-10-24 20:00 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <alpine.DEB.2.20.1610231135270.3264@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> I still do not understand (note that I am not saying "I do not
>> accept"--acceptance or rejection happens after an understandable
>> explanation is given, and "do not understand" means no such
>> explanation has been given yet) your justification behind adding a
>> technical debt to reimplement the author-script parser and not
>> sharing it with "git am" in 13/27.
>
> At this point, I am most of all reluctant to introduce such a huge change,
> which surely would introduce a regression.
That is a perfectly sensible and honest answer that I can
understand, accept and even support.
You've been working on the series for several months, running with
these dozen or so lines of code, and replacing it with another dozen
or so lines of code would require you to make sure the result is
actually doing the same thing for the remainder of your series. And
I agree that is an unnecessary risk in order to ship a working code.
The code being battle tested counts.
I cared on this point mostly because I wanted to make sure that
people later can find out why there are two functions that ought to
be doing the same thing.
If there were a technical reason why these two must stay to be
different implementations that are potentially doing different
things, I want to see that reason described, so that those who come
later and want to refactor the helper functions into one later will
know why they are separate in the first place.
If on the other hand there isn't any technical reason why they must
stay to be different, and they are different mostly because you
happened to have written a different one in 13/27 and have been
running with it, that is also a good thing for them to know.
^ permalink raw reply
* Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes
From: Lars Schneider @ 2016-10-24 19:53 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, Johannes.Schindelin, e, jnareb, gitster
In-Reply-To: <10135d1c-d9a7-d4ec-438a-bb0a8f6762fe@kdbg.org>
> On 24 Oct 2016, at 21:22, Johannes Sixt <j6t@kdbg.org> wrote:
>
> Am 24.10.2016 um 20:03 schrieb larsxschneider@gmail.com:
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> This fixes "convert: add filter.<driver>.process option" (edcc8581) on
>> Windows.
>
> Today's next falls flat on its face on Windows in t0021.15 "required process filter should filter data"; might it be the failure meant here? (I haven't dug deeper, yet.)
Yes, this is the failure meant here :-)
Cheers,
Lars
^ permalink raw reply
* Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes
From: Junio C Hamano @ 2016-10-24 19:53 UTC (permalink / raw)
To: Eric Wong; +Cc: larsxschneider, git, Johannes.Schindelin, jnareb
In-Reply-To: <20161024183900.GA12769@starla>
Eric Wong <e@80x24.org> writes:
> larsxschneider@gmail.com wrote:
>> +++ b/read-cache.c
>> @@ -156,7 +156,11 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
>> 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 (O_CLOEXEC && fd < 0 && errno == EINVAL)
>> + /* Try again w/o O_CLOEXEC: the kernel might not support it */
>> + fd = open(ce->name, O_RDONLY);
>
> In the case of O_CLOEXEC != 0 and repeated EINVALs,
> it'd be good to use something like sha1_file_open_flag as in 1/2
> so we don't repeatedly hit EINVAL. Thanks.
Sounds sane.
It's just only once, so perhaps we do not mind a recursion like
this?
read-cache.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index b594865d89..a6978b9321 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -156,11 +156,14 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
{
int match = -1;
- int fd = open(ce->name, O_RDONLY | O_CLOEXEC);
+ static int cloexec = O_CLOEXEC;
+ int fd = open(ce->name, O_RDONLY | cloexec);
- if (O_CLOEXEC && fd < 0 && errno == EINVAL)
+ if ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {
/* Try again w/o O_CLOEXEC: the kernel might not support it */
- fd = open(ce->name, O_RDONLY);
+ cloexec &= ~O_CLOEXEC;
+ return ce_compare_data(ce, st);
+ }
if (fd >= 0) {
unsigned char sha1[20];
^ permalink raw reply related
* Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"
From: Johannes Sixt @ 2016-10-24 19:47 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <CAGZ79kZrsxjSfa=CSs5V56ePabnD3-W7FHC7KkRO6jytX+GrAw@mail.gmail.com>
Am 24.10.2016 um 21:10 schrieb Stefan Beller:
> The ease of use in our own testing suite is the feature I was talking about.
Ok, thanks for the clarification. Next steps would then be, IMO, to fix
the tests to match the future world order and mark tests that fail due
to the bug with test_expect_failure, and then switch them back to
test_expect_success in the patch with the bug fix.
-- Hannes
^ permalink raw reply
* Re: [PATCH v5 00/27] Prepare the sequencer for the upcoming rebase -i patches
From: Stefan Beller @ 2016-10-24 19:36 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, git@vger.kernel.org, Jeff King,
Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <xmqqinsk8g1b.fsf@gitster.mtv.corp.google.com>
On Sat, Oct 22, 2016 at 10:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> There isn't enough time to include this topic in the upcoming
> release within the current https://tinyurl.com/gitCal calendar,
> however, which places the final on Nov 11th.
>
> I am wondering if it makes sense to delay 2.11 by moving the final
> by 4 weeks to Dec 9th.
>
> Thoughts?
>
> Speaking of what to and not to include in the upcoming release, we
> do want to include Stefan's off-by-one fix to the submodule-helper,
> but that is blocked on Windows end due to the test.
I'd be happy either way, i.e. we could revert that fix and make a release?
AFAICT, Windows only has broken tests, not broken functionality with that
submodule bug fix.
> I think
> everybody agreed that a longer time "right thing to do" fix is to
> address the "when base is /path/to/dir/., where is ../sub relative
> to it?" issue, but if we are to do so, it would need a longer
> gestation period once it hits 'next', as it can affect the current
> users and we may even need B/C notes in the release notes for the
> change. Giving ourselves a few more weeks of breathing room would
> help us to make sure the fix to relative URL issue is sound, too.
If we want a longer gestation period, we'd ideally merge it to master
just after a release, such that we "cook" it in master without having
it in any release (we had a similar discussion for the diff heuristics IIRC).
So please don't let the release schedule depend on my ability to deliver a
proper patch for the submodule path issue.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH 28/36] attr: keep attr stack for each check
From: Stefan Beller @ 2016-10-24 19:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Brandon Williams, Duy Nguyen
In-Reply-To: <xmqqmvht5zwx.fsf@gitster.mtv.corp.google.com>
On Mon, Oct 24, 2016 at 12:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Instead of having a global attr stack, attach the stack to each check.
>
> Two threads may be working on "git checkout", one "git_attr_check"
> in convert.c may be used by them to learn the EOL conversion for
> each path, and these threads are working in different parts of
> worktree in parallel. The set of .gitattributes files each of these
> threads wants to be cached at one time is tied to where in the
> directory hierarchy the thread is working in.
>
> The view by API users would not have to change from the point on
> since 27/36 or so, but I think attr_stack needs to become per
> <check, thread> tuple when we are fully thread-ready for the above
> reason.
I looked for a platform independent way to get a thread id as a natural
number, i.e. I want to get 1,2,3,... such that I could have just added
list/array of attr stacks to each check, which would be the
<check, thread> tuple you envision.
However I think we do not really need it to be per check. If we had
an easy portable way of getting such a thread id, I would have implemented
a list of stacks per thread first. (Because each thread only looks at one
check at a time.)
So this is not a baby step because I did not want to do it all at once, but
because I did not find a suitable API to use.
>
> But we need to start somewhere to move away from the current "one
> single attr stack" to "there are multiple attr stacks", and this
> "two checks may and do use different attr stacks" is probably a
> reasonable first step. It may give a single-threaded API users
> immediate benefit if the "read and keep only the entries relevant
> to the query" optimization is done with this step alone, without
> making the cache per <check, thread> pair.
>
>> This allows to use the attr in a multithreaded way.
>
> With manipulation of attr stack protected with a single Big
> Attributes Lock, this should be safe. It may not perform very well
> when used by multiple threads, though ;-)
I agree. So maybe it is not really a good fit for general consumption yet.
^ permalink raw reply
* Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes
From: Johannes Sixt @ 2016-10-24 19:22 UTC (permalink / raw)
To: larsxschneider; +Cc: git, Johannes.Schindelin, e, jnareb, gitster
In-Reply-To: <20161024180300.52359-3-larsxschneider@gmail.com>
Am 24.10.2016 um 20:03 schrieb larsxschneider@gmail.com:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> This fixes "convert: add filter.<driver>.process option" (edcc8581) on
> Windows.
Today's next falls flat on its face on Windows in t0021.15 "required
process filter should filter data"; might it be the failure meant here?
(I haven't dug deeper, yet.)
++ test_config_global filter.protocol.process
'/d/Src/mingw-git/t/t0021/rot13-filter.pl clean smudge'
++ test_when_finished 'test_unconfig --global
'\''filter.protocol.process'\'''
++ test 0 = 0
++ test_cleanup='{ test_unconfig --global '\''filter.protocol.process'\''
} && (exit "$eval_ret"); eval_ret=$?; :'
++ git config --global filter.protocol.process
'/d/Src/mingw-git/t/t0021/rot13-filter.pl clean smudge'
++ test_config_global filter.protocol.required true
++ test_when_finished 'test_unconfig --global
'\''filter.protocol.required'\'''
++ test 0 = 0
++ test_cleanup='{ test_unconfig --global '\''filter.protocol.required'\''
} && (exit "$eval_ret"); eval_ret=$?; { test_unconfig
--global '\''filter.protocol.process'\''
} && (exit "$eval_ret"); eval_ret=$?; :'
++ git config --global filter.protocol.required true
++ rm -rf repo
++ mkdir repo
++ cd repo
++ git init
Initialized empty Git repository in d:/Src/mingw-git/t/trash
directory.t0021-conversion/repo/.git/
++ echo git-stderr.log
++ echo '*.r filter=protocol'
++ git add .
++ git commit . -m 'test commit 1'
[master (root-commit) aa5dd37] test commit 1
Author: A U Thor <author@example.com>
2 files changed, 2 insertions(+)
create mode 100644 .gitattributes
create mode 100644 .gitignore
++ git branch empty-branch
++ cp 'd:/Src/mingw-git/t/trash directory.t0021-conversion/test.o' test.r
++ cp 'd:/Src/mingw-git/t/trash directory.t0021-conversion/test2.o' test2.r
++ mkdir testsubdir
++ cp 'd:/Src/mingw-git/t/trash directory.t0021-conversion/test3
'\''sq'\'',$x.o' 'testsubdir/test3 '\''sq'\'',$x.r'
+++ file_size test.r
+++ cat test.r
+++ wc -c
+++ sed 's/^[ ]*//'
++ S=57
+++ file_size test2.r
+++ cat test2.r
+++ wc -c
+++ sed 's/^[ ]*//'
++ S2=14
+++ file_size 'testsubdir/test3 '\''sq'\'',$x.r'
+++ cat 'testsubdir/test3 '\''sq'\'',$x.r'
+++ wc -c
+++ sed 's/^[ ]*//'
++ S3=49
++ filter_git add .
++ rm -f rot13-filter.log
++ git add .
error: last command exited with $?=128
not ok 15 - required process filter should filter data
^ permalink raw reply
* Re: [PATCH 27/36] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-24 19:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Brandon Williams, Duy Nguyen
In-Reply-To: <xmqqr37560gv.fsf@gitster.mtv.corp.google.com>
On Mon, Oct 24, 2016 at 11:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Make that a double-asterisk. The same problem appears in an updated
> example in technical/api-gitattributes.txt doc, but the example in
> the commit log message (below) is correct.
The implementation is actually using a double pointer, see below,
I forgot commit message and documentation
>> GIT_ATTR_RESULT_INIT_FOR(myresult, 1);
>
> Are you sure about this? We've called attr_check_initl() already so
> if this is declaring myresult, it would be decl-after-stmt.
I forgot to update the commit message and Documentation.
GIT_ATTR_RESULT_INIT_FOR is gone in the header
and in the implementation. I'll update that patch
to be consistent throughout all of {Documentation,
commit message, implementation}.
>
> The latter half is questionable. If it is "static" it wouldn't be
> thread safe, no? I think the diff in this patch for archive.c shows
> that we only expect
>
> struct git_attr_result result[2];
>
> upfront without RESULT_INIT_FOR(), and the reason why there is no
> need to free the result[] is because it is on the stack. And each
> element in result[] may point at a string, but the string belongs to
> the attr subsystem and must not be freed.
>
Same as above, it's bogus.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH 28/36] attr: keep attr stack for each check
From: Junio C Hamano @ 2016-10-24 19:07 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, pclouds
In-Reply-To: <20161022233225.8883-29-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Instead of having a global attr stack, attach the stack to each check.
Two threads may be working on "git checkout", one "git_attr_check"
in convert.c may be used by them to learn the EOL conversion for
each path, and these threads are working in different parts of
worktree in parallel. The set of .gitattributes files each of these
threads wants to be cached at one time is tied to where in the
directory hierarchy the thread is working in.
The view by API users would not have to change from the point on
since 27/36 or so, but I think attr_stack needs to become per
<check, thread> tuple when we are fully thread-ready for the above
reason.
But we need to start somewhere to move away from the current "one
single attr stack" to "there are multiple attr stacks", and this
"two checks may and do use different attr stacks" is probably a
reasonable first step. It may give a single-threaded API users
immediate benefit if the "read and keep only the entries relevant
to the query" optimization is done with this step alone, without
making the cache per <check, thread> pair.
> This allows to use the attr in a multithreaded way.
With manipulation of attr stack protected with a single Big
Attributes Lock, this should be safe. It may not perform very well
when used by multiple threads, though ;-)
^ permalink raw reply
* Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"
From: Stefan Beller @ 2016-10-24 19:10 UTC (permalink / raw)
To: Johannes Sixt
Cc: Junio C Hamano, Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <61637cd9-8f83-c988-15c0-54f948153c07@kdbg.org>
On Sun, Oct 23, 2016 at 3:14 AM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 22.10.2016 um 22:46 schrieb Stefan Beller:
>>
>> I have looked into it again, and by now I think the bug is a feature,
>> actually.
>>
>> Consider this:
>>
>> git clone . super
>> git -C super submodule add ../submodule
>> # we thought the previous line is buggy
>> git clone super super-clone
>
>
> At this point, we *should* have this if there were no bugs (at least that is
> my assumption):
>
> /tmp
> !
> + submodule <- submodule's remote repo
> !
> + foo <- we are here (.), super's remote repo
> !
> + super <- remote.origin.url=/tmp/foo/.
> !
> + submodule <- remote.origin.url=/tmp/foo/./../submodule
> submodule.submodule.url=../submodule
>
> When I test this, 'git submodule add' fails:
Yes this setup currently fails because the /tmp/foo/./../submodule
is computed to be /tmp/foo/submodule.
In the test suite the directory "foo" is usually called
"trash\ directory.tXXXX-description" and the remote of
the submodule is created inside of it, such that:
/tmp
!
+ foo <- we are here (.), super's remote repo
!
+ submodule <- submodule's remote repo
!
+ super <- remote.origin.url=/tmp/foo/.
!
+ submodule <- remote.origin.url=/tmp/foo/./../submodule
submodule.submodule.url=../submodule
current result=/tmp/foo/submodule
works.
>
> foo@master> git -C super submodule add ../submodule
> fatal: repository '/tmp/foo/submodule' does not exist
> fatal: clone of '/tmp/foo/submodule' into submodule path
> '/tmp/foo/super/submodule' failed
>
>> Now in the super-clone the ../submodule is the correct
>> relative url, because the url where we cloned from doesn't
>> end in /.
>
>
> I do not understand why this would be relevant. The question is not how the
> submodule's remote URL ends, but how the submodule's remote URL is
> constructed from the super-project's URL and the relative path specified for
> 'git submodule add'.
I was not precise here. If you have the working setup as above, you can clone
super to super-clone and it keeps working, because of the current "bug".
The difference between "super" that is cloned from . and "super-clone" that
is cloned from "super" is only the remote url, and currently
/tmp/foo/super
/tmp/foo/.
+ relative path ../submodule resolve to the same submodule remote.
>
> Whether ../submodule or ./submodule is the correct relative URL depends on
> where the origin of the submodule is located relative to the origin of the
> super-project. In the above example, it is ../submodule. However, the error
> message tells us that git looked in /tmp/foo/submodule, which looks like the
> /. bug!
Correct.
At the time I was trying to fix the urls in the test suite with the
bug fix and I then
realized that this bugfix introduces a lot of pain to our test suite,
because we do
these secondary clones quite a few times. The pattern usually is:
* setup super (cloned from .)
* clone super to super-clone
* trash super-clone for testing purposes and delete it.
>
> I do not understand where you see a feature here. What am I missing?
The ease of use in our own testing suite is the feature I was talking about.
When we would fix the bug, we would not just need to fix
s|../submodule|./submodule| when setting up super, but we would need to
fix it every time again in reverse when creating these short lived
"super-clones"
that get tested on and deleted.
So maybe the correct fix for the testing suite is a double clone, i.e. instead
of
# prepare some stuff
git clone . super
we rather do:
# mkdir upstream && prepare stuff in upstream
git clone upstream super
However that way we would end up not exercising the
code path to be fixed with the actual bug fix i.e. we'd never clone from /.
because it is silly conceptually. We could add a new test for that though
that only tests that cloning from . constructs the correct URL.
However that is already tested in t0060 resolving the relative URLs
via the git submodule helper.
Thanks for bearing this discussion,
I feel like I am missing the obvious point here,
Stefan
^ permalink raw reply
* Re: [PATCH 27/36] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-24 18:55 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, pclouds
In-Reply-To: <20161022233225.8883-28-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> This revamps the API of the attr subsystem to be thread safe.
> Before we had the question and its results in one struct type.
> The typical usage of the API was
>
> static struct git_attr_check *check;
>
> if (!check)
> check = git_attr_check_initl("text", NULL);
>
> git_check_attr(path, check);
> act_on(check->value[0]);
>
> This has a couple of issues when it comes to thread safety:
>
> * the initialization is racy in this implementation. To make it
> thread safe, we need to acquire a mutex, such that only one
> thread is executing the code in git_attr_check_initl.
> As we do not want to introduce a mutex at each call site,
> this is best done in the attr code. However to do so, we need
> to have access to the `check` variable, i.e. the API has to
> look like
> git_attr_check_initl(struct git_attr_check*, ...);
Make that a double-asterisk. The same problem appears in an updated
example in technical/api-gitattributes.txt doc, but the example in
the commit log message (below) is correct.
> The usage of the new API will be:
>
> /*
> * The initl call will thread-safely check whether the
> * struct git_attr_check has been initialized. We only
> * want to do the initialization work once, hence we do
> * that work inside a thread safe environment.
> */
> static struct git_attr_check *check;
> git_attr_check_initl(&check, "text", NULL);
>
> /*
> * Obtain a pointer to a correctly sized result
> * statically allocated on the stack; this macro:
> */
> GIT_ATTR_RESULT_INIT_FOR(myresult, 1);
Are you sure about this? We've called attr_check_initl() already so
if this is declaring myresult, it would be decl-after-stmt.
> /* Perform the check and act on it: */
> git_check_attr(path, check, myresult);
> act_on(myresult->value[0]);
>
> /*
> * No need to free the check as it is static, hence doesn't leak
> * memory. The result is also static, so no need to free there either.
> */
The latter half is questionable. If it is "static" it wouldn't be
thread safe, no? I think the diff in this patch for archive.c shows
that we only expect
struct git_attr_result result[2];
upfront without RESULT_INIT_FOR(), and the reason why there is no
need to free the result[] is because it is on the stack. And each
element in result[] may point at a string, but the string belongs to
the attr subsystem and must not be freed.
^ permalink raw reply
* Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes
From: Eric Wong @ 2016-10-24 18:39 UTC (permalink / raw)
To: larsxschneider; +Cc: git, Johannes.Schindelin, jnareb, gitster
In-Reply-To: <20161024180300.52359-3-larsxschneider@gmail.com>
larsxschneider@gmail.com wrote:
> +++ b/read-cache.c
> @@ -156,7 +156,11 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
> 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 (O_CLOEXEC && fd < 0 && errno == EINVAL)
> + /* Try again w/o O_CLOEXEC: the kernel might not support it */
> + fd = open(ce->name, O_RDONLY);
In the case of O_CLOEXEC != 0 and repeated EINVALs,
it'd be good to use something like sha1_file_open_flag as in 1/2
so we don't repeatedly hit EINVAL. Thanks.
^ permalink raw reply
* Re: Reporting Bug in Git Version Control System
From: Junio C Hamano @ 2016-10-24 18:32 UTC (permalink / raw)
To: Stefan Beller; +Cc: Yash Jain, git@vger.kernel.org
In-Reply-To: <CAGZ79ka+_CUxQjB2R-EEk2nKmc_rbj_m7884fLvU9+NgJ5gUug@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Mon, Oct 24, 2016 at 7:28 AM, Yash Jain <yashjain.lnm@gmail.com> wrote:
>> Hello,
>> I have two accounts on github("yj291197" and "yaki29").
>> Both the accounts have different gmail IDs("yj291197@gmail.com" and
>> "yashjain.lnm@gmail.com" respectively) but same passwords.
>> I used to use git for "yj291197" account and a few days earlier I made
>> this new account and used git commit to commit on "yaki29" but it
>> appeared as "yj291197" committed on "yaki29's" repo.
>> Then I pulled a request of that commit then it appeared "yaki29"
>> pulled a request with a commit of "yj291197".
>>
>> And during this whole session I was signed in as "yaki29" on github.com .
>>
>
> This is a Github issue, so ask Github support.
>
> Or read up on .mailmap files.
I am (obviously) not a GitHub support, but I think the confusion is
coming from not understanding who the committer and the author of a
commit are and where they are coming from. They are both recorded
locally, taken from user.name and user.email configuration variables
when the commits are made. "git push" to propagate them to GitHub
will NOT change these values of a commit, once a commit is created.
The story described looks quite consistent if the user has
yj291197@gmail.com configured as user.email and kept making commits
in the local repository, and pushed them to either yj291197 or yaki29
accounts at GitHub, without ever changing the local configuration to
use the other e-mail address. All commits would record the user and
e-mail address yj291197, and the only one that may be attributed to
the new one yaki29 would be the automerge created at GitHub when a
pull request is responded on-site without first fetching and making
a merge locally.
^ 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