* 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
* Re: Reporting Bug in Git Version Control System
From: Stefan Beller @ 2016-10-24 18:23 UTC (permalink / raw)
To: Yash Jain; +Cc: git@vger.kernel.org
In-Reply-To: <CAN8fUZe4iWJCZYqBBDbNyPq1Dz7f4xvTNRVEZgg5AYN2NrKCbg@mail.gmail.com>
This is a Github issue, so ask Github support.
Or read up on .mailmap files.
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 .
>
>
> Please reply ....
^ permalink raw reply
* Re: [PATCH v2 0/2] Use CLOEXEC to avoid fd leaks
From: Junio C Hamano @ 2016-10-24 18:23 UTC (permalink / raw)
To: larsxschneider; +Cc: git, Johannes.Schindelin, e, jnareb
In-Reply-To: <20161024180300.52359-1-larsxschneider@gmail.com>
larsxschneider@gmail.com writes:
> ## Changes since v1
> * add fallbacks in case O_CLOEXEC is not available
That is a good idea.
> * rename 'git_open_noatime_cloexec' to 'git_open' (Eric, Dscho)
OK. This is the old git_open_noatime() that is meant to be used
ONLY for the files Git uses for its internal implementation, and
never for end-user files. I think it is a good idea to open them
with O_CLOEXEC.
And the separate patch to use O_CLOEXEC in ce_compare_data() that
opens a working tree file for reading does not use git_open(), which
is also correct. I like it.
> * rebased the topic on `next` to fix a merge conflict
I think this applies cleanly to 'master', so that is where I'd fork
my copy at.
Thanks.
^ permalink raw reply
* Re: [PATCH 0/3] fix travis TAP/--verbose conflict
From: Lars Schneider @ 2016-10-24 18:06 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, git
In-Reply-To: <20161021104107.vh3bjx6x6pd6izat@sigill.intra.peff.net>
> On 21 Oct 2016, at 12:41, Jeff King <peff@peff.net> wrote:
>
> On Fri, Oct 21, 2016 at 04:43:48AM -0400, Jeff King wrote:
>
>> The obvious fix would be to send "--verbose" output to stderr, but I
>> suspect that would end up annoying for people who do:
>>
>> ./t5547-push-quarantine.sh -v | less
>>
>> to read long output. Probably we need some option like "--log" which
>> logs in the same way that "--tee" does, but _without_ sending the data
>> to stdout. Naively, that just means replacing the "tee" invocation with
>> "cat", but I suspect it will be a lot more complicated than that,
>> because we still need to let the TAP output go to stdout.
>
> Yeah, it was definitely a lot more complicated. This patch series fixes
> it.
Thanks a lot for this detailed and quick fix :-)
Cheers,
Lars
^ permalink raw reply
* Re: [PATCH v1 00/19] Add configuration options for split-index
From: Junio C Hamano @ 2016-10-24 18:07 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161023092648.12086-1-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> The design is similar as the previous work that introduced
> "core.untrackedCache".
>
> The new "core.splitIndex" configuration option can be either true,
> false or undefined which is the default.
>
> When it is true, the split index is created, if it does not already
> exists, when the index is read. When it is false, the split index is
> removed if it exists, when the index is read. Otherwise it is left as
> is.
I admit I haven't thought it through, but this sounds OK.
> Along with this new configuration variable, the two following options
> are also introduced:
>
> - splitIndex.maxPercentChange
>
> This is to avoid having too many changes accumulating in the split
> index while in split index mode. The git-update-index
> documentation says:
>
> If split-index mode is already enabled and `--split-index` is
> given again, all changes in $GIT_DIR/index are pushed back to
> the shared index file.
>
> but it is probably better to not expect the user to think about it
> and to have a mechanism that pushes back all changes to the shared
> index file automatically when some threshold is reached.
>
> The default threshold is when the number of entries in the split
> index file reaches 20% (by default) of the number of entries in
> the shared index file. The new "splitIndex.maxPercentChange"
> config option lets people tweak this value.
OK.
> - splitIndex.sharedIndexExpire
>
> To make sure that old sharedindex files are eventually removed
> when a new one has been created, we "touch" the shared index file
> every time it is used by a new split index file. Then we can
> delete shared indexes with an mtime older than one week (by
> default), when we create a new shared index file. The new
> "splitIndex.sharedIndexExpire" config option lets people tweak
> this grace period.
I do not quite understand this justification. Doesn't each of the
"this hold only changes since the base index file" files have a
backpointer that names the base index file it is a delta against?
Is it safe to remove a base index file when there is still a split
index file that points at it?
IOW, I do not see why it can be safe for the expiration decision to
be based on timestamp (I would understand it if it were based on a
refcnt, though).
^ permalink raw reply
* [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes
From: larsxschneider @ 2016-10-24 18:03 UTC (permalink / raw)
To: git; +Cc: Johannes.Schindelin, e, jnareb, gitster, Lars Schneider
In-Reply-To: <20161024180300.52359-1-larsxschneider@gmail.com>
From: Lars Schneider <larsxschneider@gmail.com>
This fixes "convert: add filter.<driver>.process option" (edcc8581) on
Windows.
Consider the case of a file that requires filtering and is present in
branch A but not in branch B. If A is the current HEAD and we checkout B
then the following happens:
1. ce_compare_data() opens the file
2. index_fd() detects that the file requires to run a clean filter and
calls index_stream_convert_blob()
4. index_stream_convert_blob() calls convert_to_git_filter_fd()
5. convert_to_git_filter_fd() calls apply_filter() which creates a
new long running filter process (in case it is the first file
of this kind to be filtered)
6. The new filter process inherits all file handles. This is the
default on Linux/OSX and is explicitly defined in the
`CreateProcessW` call in `mingw.c` on Windows.
7. ce_compare_data() closes the file
8. Git unlinks the file as it is not present in B
The unlink operation does not work on Windows because the filter process
has still an open handle to the file. On Linux/OSX the unlink operation
succeeds but the file descriptors still leak into the child process.
Fix this problem by opening files in read-cache with the CLOEXEC flag to
ensure that the file descriptor does not remain open in a newly spawned
process similar to 05d1ed61.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
read-cache.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/read-cache.c b/read-cache.c
index 38d67fa..b594865 100644
--- a/read-cache.c
+++ 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);
if (fd >= 0) {
unsigned char sha1[20];
--
2.10.0
^ permalink raw reply related
* [PATCH v2 1/2] sha1_file: open window into packfiles with CLOEXEC
From: larsxschneider @ 2016-10-24 18:02 UTC (permalink / raw)
To: git; +Cc: Johannes.Schindelin, e, jnareb, gitster, Lars Schneider
In-Reply-To: <20161024180300.52359-1-larsxschneider@gmail.com>
From: Lars Schneider <larsxschneider@gmail.com>
All processes that the Git main process spawns inherit the open file
descriptors of the main process. These leaked file descriptors can
cause problems.
Use the CLOEXEC flag similar to 05d1ed61 to fix the leaked file
descriptors. Since `git_open_noatime` does not describe the function
properly anymore rename it to `git_open`.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
builtin/pack-objects.c | 2 +-
cache.h | 2 +-
pack-bitmap.c | 2 +-
sha1_file.c | 26 ++++++++++++++++----------
4 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1e7c2a9..0fd52bd 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -720,7 +720,7 @@ static off_t write_reused_pack(struct sha1file *f)
if (!is_pack_valid(reuse_packfile))
die("packfile is invalid: %s", reuse_packfile->pack_name);
- fd = git_open_noatime(reuse_packfile->pack_name);
+ fd = git_open(reuse_packfile->pack_name);
if (fd < 0)
die_errno("unable to open packfile for reuse: %s",
reuse_packfile->pack_name);
diff --git a/cache.h b/cache.h
index b7f34b4..419b7a0 100644
--- a/cache.h
+++ b/cache.h
@@ -1125,7 +1125,7 @@ extern int write_sha1_file(const void *buf, unsigned long len, const char *type,
extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);
extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);
extern int force_object_loose(const unsigned char *sha1, time_t mtime);
-extern int git_open_noatime(const char *name);
+extern int git_open(const char *name);
extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);
extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);
extern int parse_sha1_header(const char *hdr, unsigned long *sizep);
diff --git a/pack-bitmap.c b/pack-bitmap.c
index b949e51..39bcc16 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -266,7 +266,7 @@ static int open_pack_bitmap_1(struct packed_git *packfile)
return -1;
idx_name = pack_bitmap_filename(packfile);
- fd = git_open_noatime(idx_name);
+ fd = git_open(idx_name);
free(idx_name);
if (fd < 0)
diff --git a/sha1_file.c b/sha1_file.c
index 1e41954..93b836b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -370,7 +370,7 @@ void read_info_alternates(const char * relative_base, int depth)
int fd;
path = xstrfmt("%s/info/alternates", relative_base);
- fd = git_open_noatime(path);
+ fd = git_open(path);
free(path);
if (fd < 0)
return;
@@ -663,7 +663,7 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)
struct pack_idx_header *hdr;
size_t idx_size;
uint32_t version, nr, i, *index;
- int fd = git_open_noatime(path);
+ int fd = git_open(path);
struct stat st;
if (fd < 0)
@@ -1069,7 +1069,7 @@ static int open_packed_git_1(struct packed_git *p)
while (pack_max_fds <= pack_open_fds && close_one_pack())
; /* nothing */
- p->pack_fd = git_open_noatime(p->pack_name);
+ p->pack_fd = git_open(p->pack_name);
if (p->pack_fd < 0 || fstat(p->pack_fd, &st))
return -1;
pack_open_fds++;
@@ -1586,9 +1586,9 @@ int check_sha1_signature(const unsigned char *sha1, void *map,
return hashcmp(sha1, real_sha1) ? -1 : 0;
}
-int git_open_noatime(const char *name)
+int git_open(const char *name)
{
- static int sha1_file_open_flag = O_NOATIME;
+ static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;
for (;;) {
int fd;
@@ -1598,12 +1598,18 @@ int git_open_noatime(const char *name)
if (fd >= 0)
return fd;
- /* Might the failure be due to O_NOATIME? */
- if (errno != ENOENT && sha1_file_open_flag) {
- sha1_file_open_flag = 0;
+ /* Try again w/o O_CLOEXEC: the kernel might not support it */
+ if (O_CLOEXEC && errno == EINVAL &&
+ (sha1_file_open_flag & O_CLOEXEC)) {
+ sha1_file_open_flag &= ~O_CLOEXEC;
continue;
}
+ /* Might the failure be due to O_NOATIME? */
+ if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {
+ sha1_file_open_flag &= ~O_NOATIME;
+ continue;
+ }
return -1;
}
}
@@ -1632,7 +1638,7 @@ static int open_sha1_file(const unsigned char *sha1)
struct alternate_object_database *alt;
int most_interesting_errno;
- fd = git_open_noatime(sha1_file_name(sha1));
+ fd = git_open(sha1_file_name(sha1));
if (fd >= 0)
return fd;
most_interesting_errno = errno;
@@ -1640,7 +1646,7 @@ static int open_sha1_file(const unsigned char *sha1)
prepare_alt_odb();
for (alt = alt_odb_list; alt; alt = alt->next) {
const char *path = alt_sha1_path(alt, sha1);
- fd = git_open_noatime(path);
+ fd = git_open(path);
if (fd >= 0)
return fd;
if (most_interesting_errno == ENOENT)
--
2.10.0
^ permalink raw reply related
* [PATCH v2 0/2] Use CLOEXEC to avoid fd leaks
From: larsxschneider @ 2016-10-24 18:02 UTC (permalink / raw)
To: git; +Cc: Johannes.Schindelin, e, jnareb, gitster, Lars Schneider
From: Lars Schneider <larsxschneider@gmail.com>
Use the CLOEXEC flag similar to 05d1ed61 to fix leaked file descriptors.
This mini patch series is necessary to make the "ls/filter-process" topic
work properly for Windows. Right now the topic generates test failures
on Windows as reported by Dscho:
http://public-inbox.org/git/alpine.DEB.2.20.1610211457030.3264@virtualbox/
Patch 1/2 was contemplated by Junio here:
http://public-inbox.org/git/xmqq37lnbbpk.fsf@gitster.mtv.corp.google.com/
Thanks to
Eric, Jakub, Dscho, and Junio for the review of v1,
Lars
## Changes since v1
* add fallbacks in case O_CLOEXEC is not available
* rename 'git_open_noatime_cloexec' to 'git_open' (Eric, Dscho)
* rebased the topic on `next` to fix a merge conflict
Lars Schneider (2):
sha1_file: open window into packfiles with CLOEXEC
read-cache: make sure file handles are not inherited by child
processes
builtin/pack-objects.c | 2 +-
cache.h | 2 +-
pack-bitmap.c | 2 +-
read-cache.c | 6 +++++-
sha1_file.c | 26 ++++++++++++++++----------
5 files changed, 24 insertions(+), 14 deletions(-)
## Interdiff (v1..v2)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 0fdd331..0fd52bd 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -720,7 +720,7 @@ static off_t write_reused_pack(struct sha1file *f)
if (!is_pack_valid(reuse_packfile))
die("packfile is invalid: %s", reuse_packfile->pack_name);
- fd = git_open_noatime_cloexec(reuse_packfile->pack_name);
+ fd = git_open(reuse_packfile->pack_name);
if (fd < 0)
die_errno("unable to open packfile for reuse: %s",
reuse_packfile->pack_name);
diff --git a/cache.h b/cache.h
index 0dea548..419b7a0 100644
--- a/cache.h
+++ b/cache.h
@@ -1125,7 +1125,7 @@ extern int write_sha1_file(const void *buf, unsigned long len, const char *type,
extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);
extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);
extern int force_object_loose(const unsigned char *sha1, time_t mtime);
-extern int git_open_noatime_cloexec(const char *name);
+extern int git_open(const char *name);
extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);
extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);
extern int parse_sha1_header(const char *hdr, unsigned long *sizep);
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 1b39e5d..39bcc16 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -266,7 +266,7 @@ static int open_pack_bitmap_1(struct packed_git *packfile)
return -1;
idx_name = pack_bitmap_filename(packfile);
- fd = git_open_noatime_cloexec(idx_name);
+ fd = git_open(idx_name);
free(idx_name);
if (fd < 0)
diff --git a/read-cache.c b/read-cache.c
index 200d4fa..b594865 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -158,6 +158,10 @@ 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);
+ 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);
+
if (fd >= 0) {
unsigned char sha1[20];
if (!index_fd(sha1, fd, st, OBJ_BLOB, ce->name, 0))
diff --git a/sha1_file.c b/sha1_file.c
index dbe027b..93b836b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -370,7 +370,7 @@ void read_info_alternates(const char * relative_base, int depth)
int fd;
path = xstrfmt("%s/info/alternates", relative_base);
- fd = git_open_noatime_cloexec(path);
+ fd = git_open(path);
free(path);
if (fd < 0)
return;
@@ -663,7 +663,7 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)
struct pack_idx_header *hdr;
size_t idx_size;
uint32_t version, nr, i, *index;
- int fd = git_open_noatime_cloexec(path);
+ int fd = git_open(path);
struct stat st;
if (fd < 0)
@@ -1069,7 +1069,7 @@ static int open_packed_git_1(struct packed_git *p)
while (pack_max_fds <= pack_open_fds && close_one_pack())
; /* nothing */
- p->pack_fd = git_open_noatime_cloexec(p->pack_name);
+ p->pack_fd = git_open(p->pack_name);
if (p->pack_fd < 0 || fstat(p->pack_fd, &st))
return -1;
pack_open_fds++;
@@ -1586,7 +1586,7 @@ int check_sha1_signature(const unsigned char *sha1, void *map,
return hashcmp(sha1, real_sha1) ? -1 : 0;
}
-int git_open_noatime_cloexec(const char *name)
+int git_open(const char *name)
{
static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;
@@ -1598,12 +1598,18 @@ int git_open_noatime_cloexec(const char *name)
if (fd >= 0)
return fd;
- /* Might the failure be due to O_NOATIME? */
- if (errno != ENOENT && sha1_file_open_flag) {
- sha1_file_open_flag = 0;
+ /* Try again w/o O_CLOEXEC: the kernel might not support it */
+ if (O_CLOEXEC && errno == EINVAL &&
+ (sha1_file_open_flag & O_CLOEXEC)) {
+ sha1_file_open_flag &= ~O_CLOEXEC;
continue;
}
+ /* Might the failure be due to O_NOATIME? */
+ if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {
+ sha1_file_open_flag &= ~O_NOATIME;
+ continue;
+ }
return -1;
}
}
@@ -1632,7 +1638,7 @@ static int open_sha1_file(const unsigned char *sha1)
struct alternate_object_database *alt;
int most_interesting_errno;
- fd = git_open_noatime_cloexec(sha1_file_name(sha1));
+ fd = git_open(sha1_file_name(sha1));
if (fd >= 0)
return fd;
most_interesting_errno = errno;
@@ -1640,7 +1646,7 @@ static int open_sha1_file(const unsigned char *sha1)
prepare_alt_odb();
for (alt = alt_odb_list; alt; alt = alt->next) {
const char *path = alt_sha1_path(alt, sha1);
- fd = git_open_noatime_cloexec(path);
+ fd = git_open(path);
if (fd >= 0)
return fd;
if (most_interesting_errno == ENOENT)
--
2.10.0
^ permalink raw reply related
* Re: [PATCH 0/4] nd/ita-empty-commit update
From: Junio C Hamano @ 2016-10-24 17:58 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <20161024104222.31128-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> The index_differs_from() also takes a flag to set/clear this new
> flag instead of relying on has_ita_entries like the old 2/3.
I think that probably is a good move.
> The name ita-invisible-in-index is not perfect but I could not think
> of any better. Another name could be diff-cached-ignores-ita, but
> that's just half of what it does. The other half is diff-files-includes-ita...
I can't either, and it is one of the reasons why I am reluctant.
Not being able to be named with a short-and-sweet name often is a
sign that the thing to be named is conceptually not well thought
out.
But as we need to give it some name to the flat to ease
experimenting, let's take that name as-is.
^ permalink raw reply
* Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"
From: Junio C Hamano @ 2016-10-24 17:46 UTC (permalink / raw)
To: Johannes Sixt
Cc: Stefan Beller, Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <61637cd9-8f83-c988-15c0-54f948153c07@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> 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:
>
> 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'.
FWIW, that matches my understanding.
> 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!
>
> I do not understand where you see a feature here. What am I missing?
>
> -- Hannes
^ permalink raw reply
* Re: [PATCH] hex: use unsigned index for ring buffer
From: Junio C Hamano @ 2016-10-24 17:27 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, Git List
In-Reply-To: <xmqqwpgx7jn6.fsf@gitster.mtv.corp.google.com>
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?
Author: René Scharfe <l.s.r@web.de>
Date: Sun Oct 23 19:57:30 2016 +0200
hex: make wraparound of the index into ring-buffer explicit
Overflow is defined for unsigned integers, but not for signed ones.
We could make the ring-buffer index in sha1_to_hex() and
get_pathname() unsigned to be on the safe side to resolve this, but
let's make it explicit that we are wrapping around at whatever the
number of elements the ring-buffer has. The compiler is smart enough
to turn modulus into bitmask for these codepaths that use
ring-buffers of a size that is a power of 2.
---
cache.h | 3 +++
hex.c | 4 ++--
path.c | 4 ++--
3 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/cache.h b/cache.h
index 4cba08ecb1..5429df0f92 100644
--- a/cache.h
+++ b/cache.h
@@ -547,6 +547,9 @@ extern int daemonize(void);
} \
} while (0)
+#define NEXT_RING_ITEM(array, index) \
+ (array)[(index) = ((index) + 1) % ARRAY_SIZE(array)]
+
/* Initialize and use the cache information */
struct lock_file;
extern int read_index(struct index_state *);
diff --git a/hex.c b/hex.c
index ab2610e498..5e711b9e32 100644
--- a/hex.c
+++ b/hex.c
@@ -76,9 +76,9 @@ char *oid_to_hex_r(char *buffer, const struct object_id *oid)
char *sha1_to_hex(const unsigned char *sha1)
{
- static int bufno;
+ static size_t bufno;
static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
- return sha1_to_hex_r(hexbuffer[3 & ++bufno], sha1);
+ return sha1_to_hex_r(NEXT_RING_ITEM(hexbuffer, bufno), sha1);
}
char *oid_to_hex(const struct object_id *oid)
diff --git a/path.c b/path.c
index fe3c4d96c6..5b2ab2271f 100644
--- a/path.c
+++ b/path.c
@@ -23,8 +23,8 @@ static struct strbuf *get_pathname(void)
static struct strbuf pathname_array[4] = {
STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
};
- static int index;
- struct strbuf *sb = &pathname_array[3 & ++index];
+ static size_t index;
+ struct strbuf *sb = &NEXT_RING_ITEM(pathname_array, index);
strbuf_reset(sb);
return sb;
}
^ permalink raw reply related
* Re: [PATCH] hex: use unsigned index for ring buffer
From: Junio C Hamano @ 2016-10-24 17:15 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, Git List
In-Reply-To: <20161024130015.awlmgpfzixiy6wkb@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> > You could also write the second line like:
>> >
>> > bufno %= ARRAY_SIZE(hexbuffer);
>> >
>> > which is less magical (right now the set of buffers must be a power of
>> > 2). I expect the compiler could turn that into a bitmask itself.
>> ...
>
> 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.
^ permalink raw reply
* Re: RFC Failover url for fetches?
From: Junio C Hamano @ 2016-10-24 16:54 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <7fdaa160-9262-5d52-7035-8362ca94beea@gmail.com>
Jakub Narębski <jnareb@gmail.com> writes:
>> As to fetching from two or more places as "fallback", I am
>> moderately negative to add it as a dumb feature that does nothing
>> more than "My fetch from A failed, so let's blindly try it from B".
>> I'd prefer to keep the "My fetch from A is failing" knowledge near
>> the surface of end user's consciousness as a mechanism to pressure A
>> to fix it--that way everybody who is fetching from A benefits.
>> After all, doing "git remote add B" once (you'd need to tell the URL
>> for B anyway to Git) and issuing "git fetch B" after seeing your
>> regular "git fetch" fails once in a blue moon is not all that
>> cumbersome, I would think.
>
> One would need to configure fallback B remote to use the same
> remote-branch namespace as remote A, if it is to be used as fallback,
> I would think.
Yeah, I left it out because I thought that was obvious, but spelling
it out explicitly may have helped those who weren't reading carefully.
Thanks
^ permalink raw reply
* [PATCH] gitk: Fix Japanese translation for "marked commit"
From: Satoshi Yasushima @ 2016-10-24 15:35 UTC (permalink / raw)
To: git; +Cc: Paul Mackerras, Satoshi Yasushima
Signed-off-by: Satoshi Yasushima <s.yasushima@gmail.com>
---
po/ja.po | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/po/ja.po b/po/ja.po
index f143753..510306b 100644
--- a/po/ja.po
+++ b/po/ja.po
@@ -2,16 +2,17 @@
# Copyright (C) 2005-2015 Paul Mackerras
# This file is distributed under the same license as the gitk package.
#
-# YOKOTA Hiroshi <yokota@netlab.cs.tsukuba.ac.jp>, 2015.
# Mizar <mizar.jp@gmail.com>, 2009.
# Junio C Hamano <gitster@pobox.com>, 2009.
+# YOKOTA Hiroshi <yokota@netlab.cs.tsukuba.ac.jp>, 2015.
+# Satoshi Yasushima <s.yasushima@gmail.com>, 2016.
msgid ""
msgstr ""
"Project-Id-Version: gitk\n"
"Report-Msgid-Bugs-To: \n"
"POT-Creation-Date: 2015-05-17 14:32+1000\n"
"PO-Revision-Date: 2015-11-12 13:00+0900\n"
-"Last-Translator: YOKOTA Hiroshi <yokota@netlab.cs.tsukuba.ac.jp>\n"
+"Last-Translator: Satoshi Yasushima <s.yasushima@gmail.com>\n"
"Language-Team: Japanese\n"
"Language: ja\n"
"MIME-Version: 1.0\n"
@@ -314,11 +315,11 @@ msgstr "マークを付けたコミットと比較する"
#: gitk:2630 gitk:2641
msgid "Diff this -> marked commit"
-msgstr "これと選択したコミットのdiffを見る"
+msgstr "これとマークを付けたコミットのdiffを見る"
#: gitk:2631 gitk:2642
msgid "Diff marked commit -> this"
-msgstr "選択したコミットとこれのdiffを見る"
+msgstr "マークを付けたコミットとこれのdiffを見る"
#: gitk:2632
msgid "Revert this commit"
--
2.10.1.windows.1
^ permalink raw reply related
* Reporting Bug in Git Version Control System
From: Yash Jain @ 2016-10-24 14:28 UTC (permalink / raw)
To: git
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 .
Please reply ....
^ permalink raw reply
* Re: [PATCH v5 00/27] Prepare the sequencer for the upcoming rebase -i patches
From: Johannes Schindelin @ 2016-10-24 14:02 UTC (permalink / raw)
To: Max Horn
Cc: Junio C Hamano, git, Stefan Beller, Jeff King,
Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <DAD768D3-5558-49DE-9FDD-E46F17933ECE@quendi.de>
Hi Max,
On Mon, 24 Oct 2016, Max Horn wrote:
> > On 23 Oct 2016, at 11:54, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Sat, 22 Oct 2016, Junio C Hamano 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.
> >
> > More is the pity.
> >
> > Thank you, though, for being upfront with me. I will shift my focus to
> > tasks that require my attention more urgently, then.
>
> Junio did go on, though:
>
> >> I am wondering if it makes sense to delay 2.11 by moving the final
> >> by 4 weeks to Dec 9th.
>
> I was reading this as an offer to delay things to accommodate the
> integration your work into 2.11. I.e. "within the current plan, there is
> no time for this, but we could adjust the plan". But maybe I am
> misinterpreting?
There is no indication that the rebase--helper patches would make it into
2.11 even with four more weeks.
I will now focus on other things that I postponed in favor of the
interactive rebase patches. In fact, I *have* to focus on some quite
pressing tasks that I neglected over those patches.
It's not like the process would magically improve just because a release
date is pushed. To the contrary, pushing the release date to allow for the
rebase--helper to be included may very well have the counterintuitive
effect of delaying things beyond even that pushed date "because there is
now so much time left" (until there isn't). It's a variation of
[Parkinson's Law](https://en.wikipedia.org/wiki/Parkinson%27s_law) ;-)
Anyway, back to work,
Dscho
^ permalink raw reply
* [RFH] limiting ref advertisements
From: Jeff King @ 2016-10-24 13:29 UTC (permalink / raw)
To: git
I'm looking into the oft-discussed idea of reducing the size of ref
advertisements by having the client say "these are the refs I'm
interested in". Let's set aside the protocol complexities for a
moment and imagine we magically have some way to communicate a set of
patterns to the server.
What should those patterns look like?
I had hoped that we could keep most of the pattern logic on the
client-side. Otherwise we risk incompatibilities between how the client
and server interpret a pattern. I had also hoped we could do some kind
of prefix-matching, which would let the server look only at the
interesting bits of the ref tree (so if you don't care about
refs/changes, and the server has some ref storage that is hierarchical,
they can literally get away without opening that sub-tree).
The patch at the end of this email is what I came up with in that
direction. It obviously won't compile without the twenty other patches
implementing transport->advertise_prefixes, but it gives you a sense of
what I'm talking about.
Unfortunately it doesn't work in all cases, because refspec sources may
be unqualified. If I ask for:
git fetch $remote master:foo
then we have to actually dwim-resolve "master" from the complete list of
refs we get from the remote. It could be "refs/heads/master",
"refs/tags/master", etc. Worse, it could be "refs/master". In that case,
at least, I think we are OK because we avoid advertising refs directly
below "refs/" in the first place. But if you have a slash, like:
git fetch $remote jk/foo
then that _could_ be "refs/jk/foo". Likewise, we cannot even optimize
the common case of a fully-qualified ref, like "refs/heads/foo". If it
exists, we obviously want to use that. But if it doesn't, then it
could be refs/something-else/refs/heads/foo. That's unlikely, but it
_does_ work now, and optimizing the advertisement would break it.
So it seems like left-anchoring the refspecs can never be fully correct.
We can communicate "master" to the server, who can then look at every
ref it would advertise and ask "could this be called master"? But it
will be setting in stone the set of "could this be" patterns. Granted,
those haven't changed much over the history of git, but it seems awfully
fragile.
In an ideal world the client and server would negotiate to come to some
agreement on the patterns being used. But as we are bolting this onto
the existing protocol, I was really trying to do it without introducing
an extra capabilities phase or extra round-trips. I.e., something like
David Turner's "stick the refspec in the HTTP query parameters" trick,
but working everywhere[1].
Clever ideas?
-Peff
[1] I do have working patches to pass these "early capabilities"
everywhere, but they're still somewhat rough. I got it to the point
where I could flip the default to "on" to see what breaks. That's
not something we'd want to do for real, but is good for running the
test suite to uncover issues like this one.
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7c10d70092..3a2585ffd7 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -302,6 +302,33 @@ static void find_non_local_tags(struct transport *transport,
string_list_clear(&remote_refs, 0);
}
+static void add_advertise_prefixes(struct transport *transport,
+ const struct refspec *refs, int nr)
+{
+ struct argv_array *list = &transport->advertise_prefixes;
+ int i;
+
+ for (i = 0; i < nr; i++) {
+ const struct refspec *rs = &refs[i];
+ size_t len;
+
+ if (!rs->pattern)
+ argv_array_push(list, rs->src);
+ else if (strip_suffix(rs->src, "/*", &len))
+ argv_array_pushf(list, "%.*s", (int)len, rs->src);
+ else {
+ /*
+ * This refspec is too complex for us to communicate;
+ * not only do we skip it, but we must avoid
+ * communicating any prefixes, since we need to see
+ * all refs.
+ */
+ transport->ignore_advertise_prefixes = 1;
+ return;
+ }
+ }
+}
+
static struct ref *get_ref_map(struct transport *transport,
struct refspec *refspecs, int refspec_count,
int tags, int *autotags)
@@ -314,12 +341,18 @@ static struct ref *get_ref_map(struct transport *transport,
/* opportunistically-updated references: */
struct ref *orefs = NULL, **oref_tail = &orefs;
- const struct ref *remote_refs = transport_get_remote_refs(transport);
+ const struct ref *remote_refs;
+
+ if (tags == TAGS_SET || (tags == TAGS_DEFAULT && *autotags))
+ add_advertise_prefixes(transport, tag_refspec, 1);
if (refspec_count) {
struct refspec *fetch_refspec;
int fetch_refspec_nr;
+ add_advertise_prefixes(transport, refspecs, refspec_count);
+ remote_refs = transport_get_remote_refs(transport);
+
for (i = 0; i < refspec_count; i++) {
get_fetch_map(remote_refs, &refspecs[i], &tail, 0);
if (refspecs[i].dst && refspecs[i].dst[0])
@@ -373,6 +406,17 @@ static struct ref *get_ref_map(struct transport *transport,
(remote->fetch_refspec_nr ||
/* Note: has_merge implies non-NULL branch->remote_name */
(has_merge && !strcmp(branch->remote_name, remote->name)))) {
+
+ add_advertise_prefixes(transport, remote->fetch,
+ remote->fetch_refspec_nr);
+ if (has_merge && !strcmp(branch->remote_name, remote->name)) {
+ int i;
+ for (i = 0; i < branch->merge_nr; i++)
+ add_advertise_prefixes(transport, branch->merge[i], 1);
+ }
+
+ remote_refs = transport_get_remote_refs(transport);
+
for (i = 0; i < remote->fetch_refspec_nr; i++) {
get_fetch_map(remote_refs, &remote->fetch[i], &tail, 0);
if (remote->fetch[i].dst &&
@@ -393,6 +437,8 @@ static struct ref *get_ref_map(struct transport *transport,
!strcmp(branch->remote_name, remote->name))
add_merge_config(&ref_map, remote_refs, branch, &tail);
} else {
+ argv_array_push(&transport->advertise_prefixes, "HEAD");
+ remote_refs = transport_get_remote_refs(transport);
ref_map = get_remote_ref(remote_refs, "HEAD");
if (!ref_map)
die(_("Couldn't find remote ref HEAD"));
^ permalink raw reply related
* Re: [PATCH] hex: use unsigned index for ring buffer
From: Jeff King @ 2016-10-24 13:00 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <fb816dd5-8fb9-c6a6-2ec2-9ea4dddfdb26@web.de>
On Sun, Oct 23, 2016 at 07:57:30PM +0200, René Scharfe wrote:
> > > Hard to trigger, but probably even harder to diagnose once someone
> > > somehow manages to do it on some uncommon architecture.
> >
> > Indeed. If we are worried about overflow, we would also want to assume
> > that it wraps at a multiple of 4, but that is probably a sane
> > assumption.
>
> Hmm, I can't think of a way to violate this assumption except with unsigned
> integers that are only a single bit wide. That would be a weird machine.
> Are there other possibilities?
No, I don't think so. I don't recall offhand whether the C standard
allows integers that are not powers of 2. But if it does, and somebody
develops such a platform, I have very little sympathy.
My comment was mostly "this is the only other restriction I can think
of, and it is crazy".
> > You could also write the second line like:
> >
> > bufno %= ARRAY_SIZE(hexbuffer);
> >
> > which is less magical (right now the set of buffers must be a power of
> > 2). I expect the compiler could turn that into a bitmask itself.
>
> Expelling magic is a good idea. And indeed, at least gcc, clang and icc on
> x86-64 are smart enough to use an AND instead of dividing
> (https://godbolt.org/g/rFPpzF).
>
> But gcc also adds a sign extension (cltq/cdqe) if we store the truncated
> value, unlike the other two compilers. I don't see why -- the bit mask
> operation enforces a value between 0 and 3 (inclusive) and the upper bits of
> eax are zeroed automatically, so the cltq is effectively a noop.
>
> Using size_t gets us rid of the extra instruction and is the right type
> anyway. It would suffice on its own, hmm..
Yeah, I had assumed you would also switch to some form of unsigned type
either way.
> > I'm fine with any of the options. I guess you'd want a similar patch for
> > find_unique_abbrev on top of jk/no-looking-at-dotgit-outside-repo.
>
> Actually I'd want you to want to amend your series yourself. ;) Maybe I can
> convince Coccinelle to handle that issue for us.
I thought that series was in "next" already, but I see it isn't yet. I'd
still wait until the sha1_to_hex() solution settles, and then copy it.
> And there's also path.c::get_pathname(). That's enough cases to justify
> adding a macro, I'd say:
> [...]
> +#define NEXT_RING_ITEM(array, index) \
> + (array)[(index) = ((index) + 1) % ARRAY_SIZE(array)]
> +
I dunno. It hides a lot of magic without saving a lot of lines in the
caller, and the callers have to make sure "array" is an array and that
"index" is unsigned.
E.g., in this code:
> @@ -24,8 +24,8 @@ static struct strbuf *get_pathname(void)
> static struct strbuf pathname_array[4] = {
> STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
> };
> - static int index;
> - struct strbuf *sb = &pathname_array[3 & ++index];
> + static size_t index;
> + struct strbuf *sb = &NEXT_RING_ITEM(pathname_array, index);
> strbuf_reset(sb);
> return sb;
> }
The truly ugly part is the repeated STRBUF_INIT. :)
I think it would be preferable to just fix it inline in each place.
-Peff
^ permalink raw reply
* Re: [PATCH] Allow stashes to be referenced by index only
From: Jeff King @ 2016-10-24 12:54 UTC (permalink / raw)
To: Aaron and Ashley Watson
Cc: git, Jon Seymour, David Caldwell, Øystein Walle,
Ævar Arnfjörð Bjarmason, David Aguilar,
Alex Henrie
In-Reply-To: <CAB0+k9JrX7Ax26HfTEgoSyj02szFyHLayqTyW6KPuxVvXBOEOw@mail.gmail.com>
On Sun, Oct 23, 2016 at 01:41:25PM -0400, Aaron and Ashley Watson wrote:
> > But what's going on here? Why did we bother running rev-parse earlier if
> > we don't actually use the value of REV?
> >
> > You mentioned tweaking it to fix a broken test, and indeed, just using
> > $REV here breaks a few tests in t3903.
> >
> > Offhand, I do not see anything wrong with pulling the non-option values
> > out in the loop. But in that case I think the assignment of REV can just
> > go away completely.
> >
>
> The only reason for REV to remain is to preserve the error message seen with
> the previous behavior. Perhaps it would be better to instead move the
> assignment
> of REV to the only place it is still used: the error message when multiple
> arguments were detected.
Ah, thanks, I missed that use. We suppress stderr, so we're literally
just getting the set of revs there. But that should match what we have
in ARGV anyway (after all, $ARGV is where we decided we had too many
revs, and what we'll feed to rev-parse to get the sha1).
So I wonder if:
Too many revisions specified: $ARGV
would be more appropriate (at which point you can probably just continue
to call it $REV).
> I'm not sure of the next steps in the process of submitting a patch.
> Should I submit a new patch by replying to this email, or is using git
> send-email to create a new mail thread better?
Generally re-rolls of a patch are done as replies to the original. Gmail
is bad about corrupting whitespace, though, so you can't just reply and
paste the patch in there. You can use ask git-send-email to continue the
thread, though:
mid=1473378397-22453-1-git-send-email-watsona4@gmail.com
git send-email --in-reply-to=$mid ...other options...
You might also want to cc the people involved in the earlier discussion.
The public-inbox archive gives a customized full send-email command for
each message to make it easy.
http://public-inbox.org/git/1473378397-22453-1-git-send-email-watsona4@gmail.com/
-Peff
^ permalink raw reply
* Re: [PATCH v5 00/27] Prepare the sequencer for the upcoming rebase -i patches
From: Max Horn @ 2016-10-24 12:24 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, git, Stefan Beller, Jeff King,
Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <alpine.DEB.2.20.1610231151140.3264@virtualbox>
Hi Dscho,
> On 23 Oct 2016, at 11:54, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> Hi Junio,
>
> On Sat, 22 Oct 2016, Junio C Hamano 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.
>
> More is the pity.
>
> Thank you, though, for being upfront with me. I will shift my focus to
> tasks that require my attention more urgently, then.
Junio did go on, though:
>> I am wondering if it makes sense to delay 2.11 by moving the final
>> by 4 weeks to Dec 9th.
I was reading this as an offer to delay things to accommodate the integration your work into 2.11. I.e. "within the current plan, there is no time for this, but we could adjust the plan". But maybe I am misinterpreting?
Cheers,
Max
^ permalink raw reply
* [PATCH 4/4] commit: don't be fooled by ita entries when creating initial commit
From: Nguyễn Thái Ngọc Duy @ 2016-10-24 10:42 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161024104222.31128-1-pclouds@gmail.com>
ita entries are dropped at tree generation phase. If the entire index
consists of just ita entries, the result would be a a commit with no
entries, which should be caught unless --allow-empty is specified. The
test "!!active_nr" is not sufficient to catch this.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/commit.c | 11 ++++++++---
t/t2203-add-intent.sh | 10 ++++++++++
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index fe8694d..42732ba 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -894,9 +894,14 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
if (amend)
parent = "HEAD^1";
- if (get_sha1(parent, sha1))
- commitable = !!active_nr;
- else {
+ if (get_sha1(parent, sha1)) {
+ int i, ita_nr = 0;
+
+ for (i = 0; i < active_nr; i++)
+ if (ce_intent_to_add(active_cache[i]))
+ ita_nr++;
+ commitable = active_nr - ita_nr > 0;
+ } else {
/*
* Unless the user did explicitly request a submodule
* ignore mode by passing a command line option we do
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 8652a96..84a9028 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -129,6 +129,16 @@ test_expect_success 'cache-tree does skip dir that becomes empty' '
)
'
+test_expect_success 'commit: ita entries ignored in empty intial commit check' '
+ git init empty-intial-commit &&
+ (
+ cd empty-intial-commit &&
+ : >one &&
+ git add -N one &&
+ test_must_fail git commit -m nothing-new-here
+ )
+'
+
test_expect_success 'commit: ita entries ignored in empty commit check' '
git init empty-subsequent-commit &&
(
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH 3/4] commit: fix empty commit creation when there's no changes but ita entries
From: Nguyễn Thái Ngọc Duy @ 2016-10-24 10:42 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161024104222.31128-1-pclouds@gmail.com>
If i-t-a entries are present and there is no change between the index
and HEAD i-t-a entries, index_differs_from() still returns "dirty, new
entries" (aka, the resulting commit is not empty), but cache-tree will
skip i-t-a entries and produce the exact same tree of current
commit.
index_differs_from() is supposed to catch this so we can abort
git-commit (unless --no-empty is specified). Update it to optionally
ignore i-t-a entries when doing a diff between the index and HEAD so
that it would return "no change" in this case and abort commit.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/commit.c | 2 +-
diff-lib.c | 4 +++-
diff.h | 2 +-
sequencer.c | 4 ++--
t/t2203-add-intent.sh | 11 +++++++++++
5 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index bb9f79b..fe8694d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -910,7 +910,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
if (ignore_submodule_arg &&
!strcmp(ignore_submodule_arg, "all"))
diff_flags |= DIFF_OPT_IGNORE_SUBMODULES;
- commitable = index_differs_from(parent, diff_flags);
+ commitable = index_differs_from(parent, diff_flags, 1);
}
}
strbuf_release(&committer_ident);
diff --git a/diff-lib.c b/diff-lib.c
index 27f1228..5244746 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -535,7 +535,8 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
return 0;
}
-int index_differs_from(const char *def, int diff_flags)
+int index_differs_from(const char *def, int diff_flags,
+ int ita_invisible_in_index)
{
struct rev_info rev;
struct setup_revision_opt opt;
@@ -547,6 +548,7 @@ int index_differs_from(const char *def, int diff_flags)
DIFF_OPT_SET(&rev.diffopt, QUICK);
DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
rev.diffopt.flags |= diff_flags;
+ rev.diffopt.ita_invisible_in_index = ita_invisible_in_index;
run_diff_index(&rev, 1);
if (rev.pending.alloc)
free(rev.pending.objects);
diff --git a/diff.h b/diff.h
index 68a6618..b171172 100644
--- a/diff.h
+++ b/diff.h
@@ -356,7 +356,7 @@ extern int diff_result_code(struct diff_options *, int);
extern void diff_no_index(struct rev_info *, int, const char **);
-extern int index_differs_from(const char *def, int diff_flags);
+extern int index_differs_from(const char *def, int diff_flags, int ita_invisible_in_index);
/*
* Fill the contents of the filespec "df", respecting any textconv defined by
diff --git a/sequencer.c b/sequencer.c
index eec8a60..b082635 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -469,7 +469,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
unborn = get_sha1("HEAD", head);
if (unborn)
hashcpy(head, EMPTY_TREE_SHA1_BIN);
- if (index_differs_from(unborn ? EMPTY_TREE_SHA1_HEX : "HEAD", 0))
+ if (index_differs_from(unborn ? EMPTY_TREE_SHA1_HEX : "HEAD", 0, 0))
return error_dirty_index(opts);
}
discard_cache();
@@ -1064,7 +1064,7 @@ static int sequencer_continue(struct replay_opts *opts)
if (ret)
return ret;
}
- if (index_differs_from("HEAD", 0))
+ if (index_differs_from("HEAD", 0, 0))
return error_dirty_index(opts);
todo_list = todo_list->next;
return pick_commits(todo_list, opts);
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 0e54f63..8652a96 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -129,5 +129,16 @@ test_expect_success 'cache-tree does skip dir that becomes empty' '
)
'
+test_expect_success 'commit: ita entries ignored in empty commit check' '
+ git init empty-subsequent-commit &&
+ (
+ cd empty-subsequent-commit &&
+ test_commit one &&
+ : >two &&
+ git add -N two &&
+ test_must_fail git commit -m nothing-new-here
+ )
+'
+
test_done
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH 2/4] diff: add --ita-[in]visible-in-index
From: Nguyễn Thái Ngọc Duy @ 2016-10-24 10:42 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161024104222.31128-1-pclouds@gmail.com>
The option --ita-invisible-in-index exposes the "ita_invisible_in_index"
diff flag to outside to allow easier experimentation with this new mode.
The "plan" is to make --ita-invisible-in-index default to keep consistent
behavior with 'status' and 'commit', but a bunch other commands like
'apply', 'merge', 'reset'.... need to be taken into consideration as well.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/diff-options.txt | 8 ++++++++
diff.c | 4 ++++
t/t2203-add-intent.sh | 4 +++-
3 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 7805a0c..0fdd53f 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -575,5 +575,13 @@ endif::git-format-patch[]
--line-prefix=<prefix>::
Prepend an additional prefix to every line of output.
+--ita-invisible-in-index::
+ By default entries added by "git add -N" appear as an existing
+ empty file in "git diff" and a new file in "git diff --cached".
+ This option makes the entry appear as a new file in "git diff"
+ and non-existent in "git diff --cached". This option could be
+ reverted with `--ita-visible-in-index`. Both options are
+ experimental and could be removed in future.
+
For more detailed explanation on these common options, see also
linkgit:gitdiffcore[7].
diff --git a/diff.c b/diff.c
index c6da383..e8e73f8 100644
--- a/diff.c
+++ b/diff.c
@@ -3923,6 +3923,10 @@ int diff_opt_parse(struct diff_options *options,
return parse_submodule_opt(options, arg);
else if (skip_prefix(arg, "--ws-error-highlight=", &arg))
return parse_ws_error_highlight(options, arg);
+ else if (!strcmp(arg, "--ita-invisible-in-index"))
+ options->ita_invisible_in_index = 1;
+ else if (!strcmp(arg, "--ita-visible-in-index"))
+ options->ita_invisible_in_index = 0;
/* misc options */
else if (!strcmp(arg, "-z"))
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 2276e4e..0e54f63 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -57,7 +57,9 @@ test_expect_success 'i-t-a entry is simply ignored' '
git add -N nitfol &&
git commit -m second &&
test $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&
- test $(git diff --name-only HEAD -- nitfol | wc -l) = 1
+ test $(git diff --name-only HEAD -- nitfol | wc -l) = 1 &&
+ test $(git diff --name-only --ita-invisible-in-index HEAD -- nitfol | wc -l) = 0 &&
+ test $(git diff --name-only --ita-invisible-in-index -- nitfol | wc -l) = 1
'
test_expect_success 'can commit with an unrelated i-t-a entry in index' '
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH 0/4] nd/ita-empty-commit update
From: Nguyễn Thái Ngọc Duy @ 2016-10-24 10:42 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20160928114348.1470-1-pclouds@gmail.com>
This version splits the old 1/3 into two, with better description in
1/4. The index_differs_from() also takes a flag to set/clear this new
flag instead of relying on has_ita_entries like the old 2/3.
The name ita-invisible-in-index is not perfect but I could not think
of any better. Another name could be diff-cached-ignores-ita, but
that's just half of what it does. The other half is diff-files-includes-ita...
Nguyễn Thái Ngọc Duy (4):
Subject: diff-lib: allow ita entries treated as "not yet exist in index"
diff: add --ita-[in]visible-in-index
commit: fix empty commit creation when there's no changes but ita entries
commit: don't be fooled by ita entries when creating initial commit
Documentation/diff-options.txt | 8 ++++++++
builtin/commit.c | 13 +++++++++----
diff-lib.c | 18 +++++++++++++++++-
diff.c | 4 ++++
diff.h | 3 ++-
sequencer.c | 4 ++--
t/t2203-add-intent.sh | 41 +++++++++++++++++++++++++++++++++++++++--
t/t7064-wtstatus-pv2.sh | 4 ++--
wt-status.c | 7 ++++++-
9 files changed, 89 insertions(+), 13 deletions(-)
--
2.8.2.524.g6ff3d78
^ permalink raw reply
* [PATCH 1/4] diff-lib: allow ita entries treated as "not yet exist in index"
From: Nguyễn Thái Ngọc Duy @ 2016-10-24 10:42 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161024104222.31128-1-pclouds@gmail.com>
When comparing the index and the working tree to show which paths are
new, and comparing the tree recorded in the HEAD and the index to see if
committing the contents recorded in the index would result in an empty
commit, we would want the former comparison to say "these are new paths"
and the latter to say "there is no change" for paths that are marked as
intent-to-add.
We made a similar attempt at d95d728a ("diff-lib.c: adjust position of
i-t-a entries in diff", 2015-03-16), which redefined the semantics of
these two comparison modes globally, which was a disaster and had to be
reverted at 78cc1a54 ("Revert "diff-lib.c: adjust position of i-t-a
entries in diff"", 2015-06-23).
To make sure we do not repeat the same mistake, introduce a new internal
diffopt option so that this different semantics can be asked for only by
callers that ask it, while making sure other unaudited callers will get
the same comparison result.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
diff-lib.c | 14 ++++++++++++++
diff.h | 1 +
t/t2203-add-intent.sh | 16 +++++++++++++++-
t/t7064-wtstatus-pv2.sh | 4 ++--
wt-status.c | 7 ++++++-
5 files changed, 38 insertions(+), 4 deletions(-)
diff --git a/diff-lib.c b/diff-lib.c
index 3007c85..27f1228 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -214,6 +214,12 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
!is_null_oid(&ce->oid),
ce->name, 0);
continue;
+ } else if (revs->diffopt.ita_invisible_in_index &&
+ ce_intent_to_add(ce)) {
+ diff_addremove(&revs->diffopt, '+', ce->ce_mode,
+ EMPTY_BLOB_SHA1_BIN, 0,
+ ce->name, 0);
+ continue;
}
changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
@@ -379,6 +385,14 @@ static void do_oneway_diff(struct unpack_trees_options *o,
struct rev_info *revs = o->unpack_data;
int match_missing, cached;
+ /* i-t-a entries do not actually exist in the index */
+ if (revs->diffopt.ita_invisible_in_index &&
+ idx && ce_intent_to_add(idx)) {
+ idx = NULL;
+ if (!tree)
+ return; /* nothing to diff.. */
+ }
+
/* if the entry is not checked out, don't examine work tree */
cached = o->index_only ||
(idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));
diff --git a/diff.h b/diff.h
index ec76a90..68a6618 100644
--- a/diff.h
+++ b/diff.h
@@ -146,6 +146,7 @@ struct diff_options {
int dirstat_permille;
int setup;
int abbrev;
+ int ita_invisible_in_index;
/* white-space error highlighting */
#define WSEH_NEW 1
#define WSEH_CONTEXT 2
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 8f22c43..2276e4e 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -5,10 +5,24 @@ test_description='Intent to add'
. ./test-lib.sh
test_expect_success 'intent to add' '
+ test_commit 1 &&
+ git rm 1.t &&
+ echo hello >1.t &&
echo hello >file &&
echo hello >elif &&
git add -N file &&
- git add elif
+ git add elif &&
+ git add -N 1.t
+'
+
+test_expect_success 'git status' '
+ git status --porcelain | grep -v actual >actual &&
+ cat >expect <<-\EOF &&
+ DA 1.t
+ A elif
+ A file
+ EOF
+ test_cmp expect actual
'
test_expect_success 'check result of "add -N"' '
diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
index 3012a4d..e319fa2 100755
--- a/t/t7064-wtstatus-pv2.sh
+++ b/t/t7064-wtstatus-pv2.sh
@@ -246,8 +246,8 @@ test_expect_success 'verify --intent-to-add output' '
git add --intent-to-add intent1.add intent2.add &&
cat >expect <<-EOF &&
- 1 AM N... 000000 100644 100644 $_z40 $EMPTY_BLOB intent1.add
- 1 AM N... 000000 100644 100644 $_z40 $EMPTY_BLOB intent2.add
+ 1 .A N... 000000 000000 100644 $_z40 $_z40 intent1.add
+ 1 .A N... 000000 000000 100644 $_z40 $_z40 intent2.add
EOF
git status --porcelain=v2 >actual &&
diff --git a/wt-status.c b/wt-status.c
index 9a14658..05a7dcb 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -437,7 +437,7 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
switch (p->status) {
case DIFF_STATUS_ADDED:
- die("BUG: worktree status add???");
+ d->mode_worktree = p->two->mode;
break;
case DIFF_STATUS_DELETED:
@@ -547,6 +547,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)
setup_revisions(0, NULL, &rev, NULL);
rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
DIFF_OPT_SET(&rev.diffopt, DIRTY_SUBMODULES);
+ rev.diffopt.ita_invisible_in_index = 1;
if (!s->show_untracked_files)
DIFF_OPT_SET(&rev.diffopt, IGNORE_UNTRACKED_IN_SUBMODULES);
if (s->ignore_submodule_arg) {
@@ -570,6 +571,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)
setup_revisions(0, NULL, &rev, &opt);
DIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);
+ rev.diffopt.ita_invisible_in_index = 1;
if (s->ignore_submodule_arg) {
handle_ignore_submodules_arg(&rev.diffopt, s->ignore_submodule_arg);
} else {
@@ -605,6 +607,8 @@ static void wt_status_collect_changes_initial(struct wt_status *s)
if (!ce_path_match(ce, &s->pathspec, NULL))
continue;
+ if (ce_intent_to_add(ce))
+ continue;
it = string_list_insert(&s->change, ce->name);
d = it->util;
if (!d) {
@@ -911,6 +915,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
init_revisions(&rev, NULL);
DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
+ rev.diffopt.ita_invisible_in_index = 1;
memset(&opt, 0, sizeof(opt));
opt.def = s->is_initial ? EMPTY_TREE_SHA1_HEX : s->reference;
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
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