* [PATCH v2] git-p4: fix git-p4.pathEncoding for removed files
From: Lars Schneider @ 2017-02-09 15:06 UTC (permalink / raw)
To: git; +Cc: luke, gitster
In-Reply-To: <CAE5ih7-=bD_ZoL5pFYfD2Qvy-XE24V_cgge0XoAvuoTK02EDfg@mail.gmail.com>
In a9e38359e3 we taught git-p4 a way to re-encode path names from what
was used in Perforce to UTF-8. This path re-encoding worked properly for
"added" paths. "Removed" paths were not re-encoded and therefore
different from the "added" paths. Consequently, these files were not
removed in a git-p4 cloned Git repository because the path names did not
match.
Fix this by moving the re-encoding to a place that affects "added" and
"removed" paths. Add a test to demonstrate the issue.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
Hi,
unfortunately, I missed to send this v2. I agree with Luke's review and
I moved the re-encode of the path name to the `streamOneP4File` and
`streamOneP4Deletion` explicitly.
Discussion:
http://public-inbox.org/git/CAE5ih7-=bD_ZoL5pFYfD2Qvy-XE24V_cgge0XoAvuoTK02EDfg@mail.gmail.com/
Thanks,
Lars
Notes:
Base Commit: 454cb6bd52 (v2.11.0)
Diff on Web: https://github.com/larsxschneider/git/commit/75ed3e92e2
Checkout: git fetch https://github.com/larsxschneider/git git-p4/fix-path-encoding-v2 && git checkout 75ed3e92e2
Interdiff (v1..v2):
diff --git a/git-p4.py b/git-p4.py
index 8f311cb4e8..dac8b4955d 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2366,15 +2366,6 @@ class P4Sync(Command, P4UserMap):
break
path = wildcard_decode(path)
- try:
- path.decode('ascii')
- except:
- encoding = 'utf8'
- if gitConfig('git-p4.pathEncoding'):
- encoding = gitConfig('git-p4.pathEncoding')
- path = path.decode(encoding, 'replace').encode('utf8', 'replace')
- if self.verbose:
- print 'Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, path)
return path
def splitFilesIntoBranches(self, commit):
@@ -2427,11 +2418,24 @@ class P4Sync(Command, P4UserMap):
self.gitStream.write(d)
self.gitStream.write('\n')
+ def encodeWithUTF8(self, path):
+ try:
+ path.decode('ascii')
+ except:
+ encoding = 'utf8'
+ if gitConfig('git-p4.pathEncoding'):
+ encoding = gitConfig('git-p4.pathEncoding')
+ path = path.decode(encoding, 'replace').encode('utf8', 'replace')
+ if self.verbose:
+ print 'Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, path)
+ return path
+
# output one file from the P4 stream
# - helper for streamP4Files
def streamOneP4File(self, file, contents):
relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
+ relPath = self.encodeWithUTF8(relPath)
if verbose:
size = int(self.stream_file['fileSize'])
sys.stdout.write('\r%s --> %s (%i MB)\n' % (file['depotFile'], relPath, size/1024/1024))
@@ -2511,6 +2515,7 @@ class P4Sync(Command, P4UserMap):
def streamOneP4Deletion(self, file):
relPath = self.stripRepoPath(file['path'], self.branchPrefixes)
+ relPath = self.encodeWithUTF8(relPath)
if verbose:
sys.stdout.write("delete %s\n" % relPath)
sys.stdout.flush()
git-p4.py | 24 ++++++++++++++----------
t/t9822-git-p4-path-encoding.sh | 16 ++++++++++++++++
2 files changed, 30 insertions(+), 10 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index fd5ca52462..dac8b4955d 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2418,11 +2418,24 @@ class P4Sync(Command, P4UserMap):
self.gitStream.write(d)
self.gitStream.write('\n')
+ def encodeWithUTF8(self, path):
+ try:
+ path.decode('ascii')
+ except:
+ encoding = 'utf8'
+ if gitConfig('git-p4.pathEncoding'):
+ encoding = gitConfig('git-p4.pathEncoding')
+ path = path.decode(encoding, 'replace').encode('utf8', 'replace')
+ if self.verbose:
+ print 'Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, path)
+ return path
+
# output one file from the P4 stream
# - helper for streamP4Files
def streamOneP4File(self, file, contents):
relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
+ relPath = self.encodeWithUTF8(relPath)
if verbose:
size = int(self.stream_file['fileSize'])
sys.stdout.write('\r%s --> %s (%i MB)\n' % (file['depotFile'], relPath, size/1024/1024))
@@ -2495,16 +2508,6 @@ class P4Sync(Command, P4UserMap):
text = regexp.sub(r'$\1$', text)
contents = [ text ]
- try:
- relPath.decode('ascii')
- except:
- encoding = 'utf8'
- if gitConfig('git-p4.pathEncoding'):
- encoding = gitConfig('git-p4.pathEncoding')
- relPath = relPath.decode(encoding, 'replace').encode('utf8', 'replace')
- if self.verbose:
- print 'Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, relPath)
-
if self.largeFileSystem:
(git_mode, contents) = self.largeFileSystem.processContent(git_mode, relPath, contents)
@@ -2512,6 +2515,7 @@ class P4Sync(Command, P4UserMap):
def streamOneP4Deletion(self, file):
relPath = self.stripRepoPath(file['path'], self.branchPrefixes)
+ relPath = self.encodeWithUTF8(relPath)
if verbose:
sys.stdout.write("delete %s\n" % relPath)
sys.stdout.flush()
diff --git a/t/t9822-git-p4-path-encoding.sh b/t/t9822-git-p4-path-encoding.sh
index 7b83e696a9..c78477c19b 100755
--- a/t/t9822-git-p4-path-encoding.sh
+++ b/t/t9822-git-p4-path-encoding.sh
@@ -51,6 +51,22 @@ test_expect_success 'Clone repo containing iso8859-1 encoded paths with git-p4.p
)
'
+test_expect_success 'Delete iso8859-1 encoded paths and clone' '
+ (
+ cd "$cli" &&
+ ISO8859="$(printf "$ISO8859_ESCAPED")" &&
+ p4 delete "$ISO8859" &&
+ p4 submit -d "remove file"
+ ) &&
+ git p4 clone --destination="$git" //depot@all &&
+ test_when_finished cleanup_git &&
+ (
+ cd "$git" &&
+ git -c core.quotepath=false ls-files >actual &&
+ test_must_be_empty actual
+ )
+'
+
test_expect_success 'kill p4d' '
kill_p4d
'
base-commit: 454cb6bd52a4de614a3633e4f547af03d5c3b640
--
2.11.0
^ permalink raw reply related
* Re: GSoC 2017: application open, deadline = February 9, 2017
From: Christian Couder @ 2017-02-09 13:17 UTC (permalink / raw)
To: Matthieu Moy
Cc: Jeff King, git, Pranit Bauva, Lars Schneider,
Carlos Martín Nieto, Johannes Schindelin, Thomas Gummerer,
Siddharth Kannan
In-Reply-To: <vpq7f4zmu3j.fsf@anie.imag.fr>
On Thu, Feb 9, 2017 at 1:22 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> I created a Git organization and invited you + Peff as admins.
Great thanks!
I accepted the invite.
> I'll
>> start cut-and-pasting to show my good faith ;-).
>
> I created this page based on last year's:
>
> https://git.github.io/SoC-2017-Org-Application/
>
> I filled-in the "org profile". "Org application" is still TODO.
I copy pasted the Org application from last year, so I think we are good.
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2017, #02; Mon, 6)
From: Michael Haggerty @ 2017-02-09 16:08 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <xmqqzihzymn3.fsf@gitster.mtv.corp.google.com>
On 02/06/2017 11:34 PM, Junio C Hamano wrote:
> [...]
> --------------------------------------------------
> [Stalled]
> [...]
> * mh/ref-remove-empty-directory (2017-01-07) 23 commits
> - files_transaction_commit(): clean up empty directories
> - try_remove_empty_parents(): teach to remove parents of reflogs, too
> - try_remove_empty_parents(): don't trash argument contents
> - try_remove_empty_parents(): rename parameter "name" -> "refname"
> - delete_ref_loose(): inline function
> - delete_ref_loose(): derive loose reference path from lock
> - log_ref_write_1(): inline function
> - log_ref_setup(): manage the name of the reflog file internally
> - log_ref_write_1(): don't depend on logfile argument
> - log_ref_setup(): pass the open file descriptor back to the caller
> - log_ref_setup(): improve robustness against races
> - log_ref_setup(): separate code for create vs non-create
> - log_ref_write(): inline function
> - rename_tmp_log(): improve error reporting
> - rename_tmp_log(): use raceproof_create_file()
> - lock_ref_sha1_basic(): use raceproof_create_file()
> - lock_ref_sha1_basic(): inline constant
> - raceproof_create_file(): new function
> - safe_create_leading_directories(): set errno on SCLD_EXISTS
> - safe_create_leading_directories_const(): preserve errno
> - t5505: use "for-each-ref" to test for the non-existence of references
> - refname_is_safe(): correct docstring
> - files_rename_ref(): tidy up whitespace
>
> Deletion of a branch "foo/bar" could remove .git/refs/heads/foo
> once there no longer is any other branch whose name begins with
> "foo/", but we didn't do so so far. Now we do.
>
> Expecting a reroll.
> cf. <5051c78e-51f9-becd-e1a6-9c0b781d6912@alum.mit.edu>
I think you missed v4 of this patch series [1], which is the re-roll
that you were waiting for. And I missed that you missed it...
Michael
[1] http://public-inbox.org/git/cover.1483719289.git.mhagger@alum.mit.edu/
^ permalink raw reply
* Re: [RFD] should all merge bases be equal?
From: Junio C Hamano @ 2017-02-09 16:57 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git
In-Reply-To: <8a9b3f20-eed2-c59b-f7ea-3c68b3c30bf5@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> Anyway, I mostly wanted to remind you of the earlier discussion of this
> topic. There's a lot more information there.
>
> Michael
>
> [1] http://public-inbox.org/git/539A25BF.4060501@alum.mit.edu/
Your http://public-inbox.org/git/53A3F67A.80501@alum.mit.edu/ in the
thread appears to me the best place to continue exploring.
Thanks for the link.
^ permalink raw reply
* Re: [PATCH 1/5] refs: store submodule ref stores in a hashmap
From: Stefan Beller @ 2017-02-09 16:58 UTC (permalink / raw)
To: Michael Haggerty
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Johannes Schindelin, David Turner, git@vger.kernel.org
In-Reply-To: <a944446c4c374125082f5ad8b79e731704b66196.1486629195.git.mhagger@alum.mit.edu>
> @@ -1402,17 +1435,17 @@ struct ref_store *ref_store_init(const char *submodule)
>
> struct ref_store *lookup_ref_store(const char *submodule)
> {
> + if (!submodule_ref_stores.tablesize)
> + hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 20);
So we can lookup a submodule even before we initialized the subsystem?
Does that actually happen? (It sounds like a bug to me.)
Instead of initializing, you could return NULL directly here.
Otherwise looks good.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH 2/5] refs: push the submodule attribute down
From: Stefan Beller @ 2017-02-09 17:03 UTC (permalink / raw)
To: Michael Haggerty
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Johannes Schindelin, David Turner, git@vger.kernel.org
In-Reply-To: <8958e7e26cc8bf11a76672eb8ea98bc9ba662fdc.1486629195.git.mhagger@alum.mit.edu>
On Thu, Feb 9, 2017 at 5:26 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Push the submodule attribute down from ref_store to files_ref_store.
> This is another step towards loosening the 1:1 connection between
> ref_stores and submodules.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Looks good,
Stefan
^ permalink raw reply
* Re: [PATCH v3 00/27] Revamp the attribute system; another round
From: Brandon Williams @ 2017-02-09 17:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, pclouds
In-Reply-To: <xmqqwpd8s8vd.fsf@gitster.mtv.corp.google.com>
On 02/02, Junio C Hamano wrote:
> prepare the <question, answer> array
> for path taken from some set:
> do_something(the array, path)
>
> That way, do_something() do not have to keep allocating,
> initializing and destroying the array.
>
> But after looking at the current set of codepaths, before coming to
> the conclusion that we need to split the static part that is
> specific to the callsite for git_check_attr() and the dynamic part
> that is specific to the <callsite, thread> pair, I noticed that
> typically the callers that can prepare the array before going into
> the loop (which will eventually be spread across multiple threads)
> are many levels away in the callchain, and they are not even aware
> of what attributes are going to be requested in the leaf level
> helper functions. In other words, the approach to hoist "the
> <question, answer> array" up in the callchain would not scale. A
> caller that loops over paths in the index and check them out does
> not want to know (and we do not want to tell it) what exact
> attributes are involved in the decision convert_to_working_tree()
> makes for each path, for example.
This was something that I was envisioning as well, though I didn't dig
very deep into the call stack. Another means of doing this could be to
have the attr_check structure allocated and then have it configured at a
later point for the particular question being asked:
alloc struct attr_check c;
... many call sites down
configure(c, questions)
for path
do_something(c, path)
That also allows the same structure to be reused (just reconfigured) if
different attributes are needed at a later point in time. Of course
this is just an idea and I'm not sure if this is the best way to do it
either.
>
> So how would we split questions and answers in a way that is not
> naive and inefficient?
>
> I envision that we would allow the attribute subsystem to keep track
> of the dynamic part, which will receive the answers, holds working
> area like check_all_attr[], and has the equivalent to the "attr
> stack", indexed by <thread-id, callsite> pair (and the
> identification of "callsite" can be done by using the address of the
> static part, i.e. the array of questions that we initialize just
> once when interning the list of attribute names for the first time).
>
> The API to prepare and ask for attributes may look like:
>
> static struct attr_static_part Q;
> struct attr_dynamic_part *D;
>
> attr_check_init(&Q, "text", ...);
> D = git_attr_check(&Q, path);
>
> where Q contains an array of interned attributes (i.e. questions)
> and other immutable things that is unique to this callsite, but can
> be shared across multiple threads asking the same question from
> here. As an internal implementation detail, it probably will have a
> mutex to make sure that init will run only once.
>
> Then the implementation of git_attr_check(&Q, path) would be:
>
> - see if there is already the "dynaic part" allocated for the
> current thread asking the question Q. If there is not,
> allocate one and remember it, so that it can be reused in
> later calls by the same thread; if there is, use that existing
> one.
>
> - reinitialize the "dynamic part" as needed, e.g. clear the
> equivalent to check_all_attr[], adjust the equivalent to
> attr_stack for the current path, etc. Just like the current
> code optimizes for the case where the entire program (a single
> thread) will ask the same question for paths in traversal
> order (i.e. falling in the same directory), this will optimize
> for the access pattern where each thread asks the same
> question for paths in its traversal order.
>
> - do what the current collect_some_attrs() thing does.
>
> And this hopefully won't be as costly as the naive and inefficient
> one.
I agree, this sort of implementation wouldn't suffer from the same
allocation penalty that the naive implementation suffers from. This
would be slightly challenging to ensure that there aren't any memory
leaks, well not leaks but rather memory that isn't freed. i.e. When a
thread terminates we would want to reclaim the memory used for the
dynamic part which is stored inside the attribute system.
>
> The reason why I was pushing hard to split the static part and the
> dynamic part in our redesign of the API is primarily because I didn't
> want to update the API callers twice. But I'd imagine that your v3
> (and your earlier "do not discard attr stack, but keep them around,
> holding their tips in a hashmap for quick reuse") would at least lay
> the foundation for the eventual shape of the API, let's bite the
> bullet and accept that we will need to update the callers again
> anyway.
>
> Thanks.
>
At least v3 gets the attribute system to a state where further
improvements should be relatively easy to make. And now as long as each
thread has a unique attr_check structure, multiple callers can exist
inside the attribute system at the same time. There is still more work
to be done on it though. Still my biggest complaint is the "direction"
aspect of the system. I would love to also eliminate that as global
state at some point though I'm not sure how at this point.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 4/5] files_ref_store::submodule: use NULL for the main repository
From: Stefan Beller @ 2017-02-09 17:25 UTC (permalink / raw)
To: Michael Haggerty
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Johannes Schindelin, David Turner, git@vger.kernel.org
In-Reply-To: <111d663c0fd3e9669e7c28537f581833488ca4a6.1486629195.git.mhagger@alum.mit.edu>
On Thu, Feb 9, 2017 at 5:27 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> The old practice of storing the empty string in this member for the main
> repository was a holdover from before 00eebe3 (refs: create a base class
> "ref_store" for files_ref_store, 2016-09-04), when the submodule was
> stored in a flex array at the end of `struct files_ref_store`. Storing
> NULL for this case is more idiomatic and a tiny bit less code.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
Makes sense!
Stefan
^ permalink raw reply
* Re: Automatically Add .gitignore Files
From: Thangalin @ 2017-02-09 17:25 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8DoXCNzryQusEcXtOgeU2ZH1FMGEK32z5b=PGkfvJ0BTg@mail.gmail.com>
Hi Duy,
> This is a general problem to new files, not .gitignore alone. Can we
The difference, to me, is that a ".gitignore" file is not part of what
I'm developing. It's an artifact for git configuration. While a
.gitignore file is not always pushed to the repository, I imagine that
in most situations, it is.
Whereas when a "new" file is created, there are plenty of situations
where it shouldn't be added and thus a warning would be superfluous,
or an automatic add would be undesirable.
To solve the problem, generally, for new files while giving the user
the ability to specify exactly what "new" files should be
automatically added to the repository, something like the following
would work:
echo "**/.gitignore" >> .git/config/add-before-commit
> and perhaps you want to make it a habit to run it before committing.
Right, because software shouldn't automate repetitive tasks and humans
are never prone to forget? ;-)
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2017, #02; Mon, 6)
From: Junio C Hamano @ 2017-02-09 17:20 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <alpine.DEB.2.20.1702091319350.3496@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> (And that would have to be handled at a different point, as I
> had pointed out, so that suggested preparation would most likely not help
> at all.)
I did not think "that would have to be handled at a different point"
is correct at all, if by "a point" you meant "a location in the
code" [*1*]. If we want to make it configurable in a more detailed
way by directly allowing to override port_option and needs_batch
separately, you would do that in override_ssh_variant(), without
touching handle_ssh_variant() in the refactored code. That way, you
do not have to worry about breaking the auto detection based on the
command name.
[Footnote]
*1* Or did you mean "point in time", aka "let's do it outside the
scope of this patch series"?
Let's not keep it as a SQUASH??? proposal, but as a separate hot-fix
follow-up patch.
-- >8 --
Subject: connect.c: stop conflating ssh command names and overrides
dd33e07766 ("connect: Add the envvar GIT_SSH_VARIANT and ssh.variant
config", 2017-02-01) attempted to add support for configuration and
environment variable to override the different handling of
port_option and needs_batch settings suitable for variants of the
ssh implementation that was autodetected by looking at the ssh
command name. Because it piggybacked on the code that turns command
name to specific override (e.g. "plink.exe" and "plink" means
port_option needs to be set to 'P' instead of the default 'p'), yet
it defined a separate namespace for these overrides (e.g. "putty"
can be usable to signal that port_option needs to be 'P'), however,
it made the auto-detection based on the command name less robust
(e.g. the code now accepts "putty" as a SSH command name and applies
the same override).
Separate the code that interprets the override that was read from
the configuration & environment from the original code that handles
the command names, as they are in separate namespaces, to fix this
confusion.
This incidentally also makes it easier for future enhancement of the
override syntax (e.g. "port_option=p,needs_batch=1" may want to be
accepted as a more explicit syntax) without affecting the code for
auto-detection based on the command name.
While at it, update the return type of the handle_ssh_variant()
helper function to void; the caller does not use it, and the
function does not return any meaningful value.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
connect.c | 45 ++++++++++++++++++++++++++++++++-------------
1 file changed, 32 insertions(+), 13 deletions(-)
diff --git a/connect.c b/connect.c
index 7f1f802396..7d65c1c736 100644
--- a/connect.c
+++ b/connect.c
@@ -691,17 +691,39 @@ static const char *get_ssh_command(void)
return NULL;
}
-static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
- int *port_option, int *needs_batch)
+static int override_ssh_variant(int *port_option, int *needs_batch)
{
- const char *variant = getenv("GIT_SSH_VARIANT");
+ char *variant;
+
+ variant = xstrdup_or_null(getenv("GIT_SSH_VARIANT"));
+ if (!variant &&
+ git_config_get_string("ssh.variant", &variant))
+ return 0;
+
+ if (!strcmp(variant, "plink") || !strcmp(variant, "putty")) {
+ *port_option = 'P';
+ *needs_batch = 0;
+ } else if (!strcmp(variant, "tortoiseplink")) {
+ *port_option = 'P';
+ *needs_batch = 1;
+ } else {
+ *port_option = 'p';
+ *needs_batch = 0;
+ }
+ free(variant);
+ return 1;
+}
+
+static void handle_ssh_variant(const char *ssh_command, int is_cmdline,
+ int *port_option, int *needs_batch)
+{
+ const char *variant;
char *p = NULL;
- if (variant)
- ; /* okay, fall through */
- else if (!git_config_get_string("ssh.variant", &p))
- variant = p;
- else if (!is_cmdline) {
+ if (override_ssh_variant(port_option, needs_batch))
+ return;
+
+ if (!is_cmdline) {
p = xstrdup(ssh_command);
variant = basename(p);
} else {
@@ -717,12 +739,11 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
*/
free(ssh_argv);
} else
- return 0;
+ return;
}
if (!strcasecmp(variant, "plink") ||
- !strcasecmp(variant, "plink.exe") ||
- !strcasecmp(variant, "putty"))
+ !strcasecmp(variant, "plink.exe"))
*port_option = 'P';
else if (!strcasecmp(variant, "tortoiseplink") ||
!strcasecmp(variant, "tortoiseplink.exe")) {
@@ -730,8 +751,6 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
*needs_batch = 1;
}
free(p);
-
- return 1;
}
/*
--
2.12.0-rc0-221-g3e954cf1aa
^ permalink raw reply related
* Re: [PATCH 3/5] register_ref_store(): new function
From: Stefan Beller @ 2017-02-09 17:20 UTC (permalink / raw)
To: Michael Haggerty
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Johannes Schindelin, David Turner, git@vger.kernel.org
In-Reply-To: <ce326e17822184eff434b957d28f2233795162db.1486629195.git.mhagger@alum.mit.edu>
On Thu, Feb 9, 2017 at 5:27 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Move the responsibility for registering the ref_store for a submodule
> from base_ref_store_init() to a new function, register_ref_store(). Call
> the latter from ref_store_init().
>
> This means that base_ref_store_init() can lose its submodule argument,
> further weakening the 1:1 relationship between ref_stores and
> submodules.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> +
> struct ref_store *ref_store_init(const char *submodule)
> {
> const char *be_name = "files";
> struct ref_storage_be *be = find_ref_storage_backend(be_name);
> + struct ref_store *refs;
>
> if (!be)
> die("BUG: reference backend %s is unknown", be_name);
>
> if (!submodule || !*submodule)
> - return be->init(NULL);
> + refs = be->init(NULL);
> else
> - return be->init(submodule);
> + refs = be->init(submodule);
> +
> + register_ref_store(refs, submodule);
> + return refs;
> }
This function is already very readable, though maybe it would be
more readable like so:
{
const char *be_name = "files";
struct ref_storage_be *be = find_ref_storage_backend(be_name);
if (!be)
die("BUG: reference backend %s is unknown", be_name);
/* replace empty string by NULL */
if (submodule && !*submodule)
submodule = NULL;
register_ref_store(be->init(submodule), submodule);
return refs;
}
Well, I dunno; the function inside the arguments to register seems ugly, though.
^ permalink raw reply
* Re: [PATCH 1/5] refs: store submodule ref stores in a hashmap
From: Michael Haggerty @ 2017-02-09 17:40 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Johannes Schindelin, David Turner, git@vger.kernel.org
In-Reply-To: <CAGZ79kau2bYs7zegEiacAdbhn1LyOfAH9__rePfbQkX2iLgmMQ@mail.gmail.com>
On 02/09/2017 05:58 PM, Stefan Beller wrote:
>> @@ -1402,17 +1435,17 @@ struct ref_store *ref_store_init(const char *submodule)
>>
>> struct ref_store *lookup_ref_store(const char *submodule)
>> {
>
>> + if (!submodule_ref_stores.tablesize)
>> + hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 20);
>
>
> So we can lookup a submodule even before we initialized the subsystem?
> Does that actually happen? (It sounds like a bug to me.)
>
> Instead of initializing, you could return NULL directly here.
The lines you quoted are only concerned with bringing the (empty)
hashmap into existence if it hasn't been initialized already. (There's
no HASHMAP_INIT.) I don't know what you mean by "initialize the
subsystem". The only way to bring a ref_store *object* into existence is
currently to call get_ref_store(submodule), which calls
lookup_ref_store(submodule) to see if it already exists, and if not
calls ref_store_init(submodule) to instantiate it and register it in the
hashmap. There's nothing else that has to be initialize before that
(except maybe the usual startup config reading etc.)
I suppose this code path could be changed to return NULL without
initializing the hashmap, but the hashmap will be initialized a moment
later by ref_store_init(), so I don't see much difference either way.
Thanks for your review!
Michael
^ permalink raw reply
* Re: [PATCH 1/5] refs: store submodule ref stores in a hashmap
From: Stefan Beller @ 2017-02-09 17:43 UTC (permalink / raw)
To: Michael Haggerty
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Johannes Schindelin, David Turner, git@vger.kernel.org
In-Reply-To: <4a21dba7-76ef-6aec-b326-c1046f3daad2@alum.mit.edu>
On Thu, Feb 9, 2017 at 9:40 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 02/09/2017 05:58 PM, Stefan Beller wrote:
>>> @@ -1402,17 +1435,17 @@ struct ref_store *ref_store_init(const char *submodule)
>>>
>>> struct ref_store *lookup_ref_store(const char *submodule)
>>> {
>>
>>> + if (!submodule_ref_stores.tablesize)
>>> + hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 20);
>>
>>
>> So we can lookup a submodule even before we initialized the subsystem?
>> Does that actually happen? (It sounds like a bug to me.)
>>
>> Instead of initializing, you could return NULL directly here.
>
> The lines you quoted are only concerned with bringing the (empty)
> hashmap into existence if it hasn't been initialized already. (There's
> no HASHMAP_INIT.) I don't know what you mean by "initialize the
> subsystem". The only way to bring a ref_store *object* into existence is
> currently to call get_ref_store(submodule), which calls
> lookup_ref_store(submodule) to see if it already exists, and if not
> calls ref_store_init(submodule) to instantiate it and register it in the
> hashmap. There's nothing else that has to be initialize before that
> (except maybe the usual startup config reading etc.)
>
> I suppose this code path could be changed to return NULL without
> initializing the hashmap, but the hashmap will be initialized a moment
> later by ref_store_init(), so I don't see much difference either way.
Oh, I did not see that.
Thanks,
Stefan
>
> Thanks for your review!
> Michael
>
^ permalink raw reply
* Re: [PATCH 5/5] read_loose_refs(): read refs using resolve_ref_recursively()
From: Stefan Beller @ 2017-02-09 17:39 UTC (permalink / raw)
To: Michael Haggerty
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Johannes Schindelin, David Turner, git@vger.kernel.org
In-Reply-To: <024d6b2e5ca1ffa876c2911e6d9d0bb4f6091730.1486629195.git.mhagger@alum.mit.edu>
On Thu, Feb 9, 2017 at 5:27 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> There is no need to call read_ref_full() or resolve_gitlink_ref() from
> read_loose_refs(), because we already have a ref_store object in hand.
> So we can call resolve_ref_recursively() ourselves. Happily, this
> unifies the code for the submodule vs. non-submodule cases.
>
> This requires resolve_ref_recursively() to be exposed to the refs
> subsystem, though not to non-refs code.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
Looks good,
Stefan
^ permalink raw reply
* Re: [PATCH 3/5] register_ref_store(): new function
From: Michael Haggerty @ 2017-02-09 18:14 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Johannes Schindelin, David Turner, git@vger.kernel.org
In-Reply-To: <CAGZ79kYrqfmNGE0A63iYaW=MSFwANRXnn3kkxHE8kpOtF2KeLA@mail.gmail.com>
On 02/09/2017 06:20 PM, Stefan Beller wrote:
> On Thu, Feb 9, 2017 at 5:27 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> Move the responsibility for registering the ref_store for a submodule
>> from base_ref_store_init() to a new function, register_ref_store(). Call
>> the latter from ref_store_init().
>>
>> This means that base_ref_store_init() can lose its submodule argument,
>> further weakening the 1:1 relationship between ref_stores and
>> submodules.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>
>
>
>
>> +
>> struct ref_store *ref_store_init(const char *submodule)
>> {
>> const char *be_name = "files";
>> struct ref_storage_be *be = find_ref_storage_backend(be_name);
>> + struct ref_store *refs;
>>
>> if (!be)
>> die("BUG: reference backend %s is unknown", be_name);
>>
>> if (!submodule || !*submodule)
>> - return be->init(NULL);
>> + refs = be->init(NULL);
>> else
>> - return be->init(submodule);
>> + refs = be->init(submodule);
>> +
>> + register_ref_store(refs, submodule);
>> + return refs;
>> }
>
> This function is already very readable, though maybe it would be
> more readable like so:
>
> {
> const char *be_name = "files";
> struct ref_storage_be *be = find_ref_storage_backend(be_name);
>
> if (!be)
> die("BUG: reference backend %s is unknown", be_name);
>
> /* replace empty string by NULL */
> if (submodule && !*submodule)
> submodule = NULL;
>
> register_ref_store(be->init(submodule), submodule);
> return refs;
> }
>
> Well, I dunno; the function inside the arguments to register seems ugly, though.
Nit: you forgot to define and initialize `refs` (for returning to the
caller).
Actually, there is an inconsistency between the docstring for this
function and its behavior. The docstring claims that it can handle
`submodule == ""`, and it tries to do so, but incorrectly. The problem
is that it passes an un-cleaned-up `submodule` to
`register_ref_store()`, which is expecting a cleaned-up one.
But this function is only called by get_ref_store(), which has already
arranged for the empty string to be passed along as NULL.
In fact, the only external entry point into these functions is
`get_ref_store()`. I think what I should do is make the other functions
private, and remove their attempts to handle `submodule == ""`. I'll fix
this up in a re-roll.
(I wonder whether anybody really passes the empty string into this
method. It never happens in the test suite. I doubt I can muster the
energy to audit all of the call paths.)
Michael
^ permalink raw reply
* Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"
From: Siddharth Kannan @ 2017-02-09 18:21 UTC (permalink / raw)
To: Matthieu Moy
Cc: Junio C Hamano, Git List, Pranit Bauva, Jeff King, pclouds,
brian m. carlson
In-Reply-To: <vpqwpczlfe5.fsf@anie.imag.fr>
On 9 February 2017 at 17:55, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
>
>> [...]
>> As you might notice, in this list, most commands are not of the `rm` variety,
>> i.e. something that would delete stuff.
>
> OK, I think I'm convinced.
I am glad! :)
>
> Keep the arguments in mind when polishing the commit message.
I will definitely do that. I am working on a good commit message for
this by looking at some past changes to sha1_name.c which have
affected multiple commands.
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
--
Best Regards,
- Siddharth.
^ permalink raw reply
* Re: Google Doc about the Contributors' Summit
From: Junio C Hamano @ 2017-02-09 19:09 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
In-Reply-To: <alpine.DEB.2.20.1702021007460.3496@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> I just started typing stuff up in a Google Doc, and made it editable to
> everyone, feel free to help and add things:
>
> https://docs.google.com/document/d/1KDoSn4btbK5VJCVld32he29U0pUeFGhpFxyx9ZJDDB0/edit?usp=sharing
Thanks for a write-up, Dscho.
List of bullet points just make non-attendees envious, imagining
that attendees had all the fun discussing what is behind these
bullet points, without being able to know what was discussed, if
they reached consensus and what the consensus was, but it is OK ;-)
A few items caught my attention, not because I found them more
important than other items on the page, but because they seem to
want my input without directly asking me ;-)
* If Junio would accept patches by replying to the sender with an
ID and/or a patch name. He picks this (branch) name when he gets
your patch.
I am not sure what exactly I am asked to "accept" here. I sometimes
forget to respond with "Thanks, will queue." to the patch message
and whoever said this wants me to consistently do so? I never say
"... will queue as js/difftool-builtin topic." mainly because I do
not know what the name of the topic branch should be at the point of
reading the patch. All I know is that decided that it may be worth
queuing, so it is a bit harder to arrange. But I can certainly try
if it makes the lives of contributors easier.
* Junio has a script for the todo branch which we can use to
generate the what's cooking branch. Perhaps we could continuously
generate this onto a webpage.
I have no objection, but I doubt that people find such an auto
generated version all that useful, as "git log --first-parent
origin/master..origin/pu" would tell exactly the same story. It
will lack the "topic summary" comment I have yet to write, if the
final 'pu' of the day that was pushed out was before my local update
to the draft of the next issue of "What's cooking" report [*1*], and
would not have any update on the next action (e.g. "Will merge to
...") relative to the latest issue of "What's cooking" report. IOW,
such an auto-generated report lacks all the added value over "git log"
output.
* Making the actual workflow more publicly known, e.g. document how
to generate the cooking email, to learn about the state of a
generation.
The exact mechanics of "how to generate" may be of less importance
than "how the information contained therein relates to their own
work" to the contributors, and I think MaintNotes that is sent out
at key milestones more or less covers the mechanics, but here is how
the sausage is made these days:
- I find a patch series on the list that is in good enough shape to
be in 'pu'. Perhaps it was already discussed and redone a few
times without hitting my tree. Or it may be the first attempt of
a new topic. I come up with a topic name, decide where the topic
should fork at [*2*], create the topic branch and queue the
patches. I may or may not test the topic in isolation at this
point.
- I may find an updated patch series of what has been queued. I go
to the existing topic branch and replace it (I try to keep it
forked at the same commit) after inspecting what got updated.
"git tbdiff" is a useful program to help this step. I may or may
not test the topic in isolation at this point.
- I repeat the above two for various topics during the day, and at
some point between 14:00-15:00 my time, stop taking new patches.
The day's integration cycle starts.
- If there are topics that have cooked long enough in 'next' and
planned to graduate to 'master', merge [*3*] them to 'master',
update the draft Release notes. Otherwise skip this step.
- If there are topics that have cooked long enough in 'pu' and
planned to graduate to 'next', merge them to 'next'. Otherwise
skip this step.
- If I updated 'master', merge its tip to 'next' (this should update
the draft release notes and nothing else, unless I took something
directly to 'master').
- Then I recreate 'jch', which is a point between 'master' and 'pu'.
This is the version I use for my own work, contains all topics in
'next' but a bit more. "git checkout -B jch master" begins it,
and then the topics that were in 'jch' are merged on top. The
latest version of updated topics that were already in 'jch' are
incorporated at this point, and "git diff jch@{20.hours}" would
show the effect of their interdiff (plus RelNotes update, if
'master' was updated during this integration cycle).
- I ran "git branch --no-merged pu --sort=-committerdate" to see the
topics that are new; the top of this list shows new topics and
updated topics (note that I just updated 'jch' but not 'pu' yet at
this point). Among them, I pick the ones that I am willing to be
a guinea-pig for before they hit 'next' and merge them to 'jch'.
Other topics that used to be in 'pu' may also be merged at this
point, when they turn out to be "interesting" enough.
- 'pu' is recreated on top of 'jch' [*4*]. "git checkout -B pu jch"
begins it, the topics that were already in 'pu' are merged on top
(some of them may have been merged to updated 'jch' in the above
step), together with the new topics we acquired.
- All integration branches are then tested (and installed for my own
use) at this point.
- While the tests are running, the draft "What's cooking" report is
updated. Here is where I write topic summary for new topics and
update topic summary for updated topics. The next action
(e.g. "Will merge to ...") may also be updated at this point.
- If the test notices problems, I go back to redo the day's merges
[*5*]. The first thing to do is to rewind the integration
branches back to the state before the day's integration started.
Trivial breakages are fixed-up in place with "rebase -i" or
queuing "SQUASH???" fix on top of the offending topic before
merging them back to 'master', 'next', 'jch' or 'pu'. A more
involved ones may force me to punt and eject the topic from the
integration branches. Or I may leave it in 'pu' so that others
who are more clueful in the area the offending topic touches can
notice the breakage and send in fix-up patches [*6*].
- If I have time, I redo 'jch' and 'pu' at this point once again,
because the merge commit for new topics we just tested do not have
their merge log message in sync with the topic summary in the
draft "What's cooking" report at this point [*6*]. I run the
tests for integration branches again, but the tree objects at the
tips of the integration branches are expected to be the same as
what was tested earlier, so it becomes a no-op.
- I push out the integration branches to the usual places [*7*].
- Then I go back to the very beginning. I start taking patches for
the next day's integration cycle.
Interested people are welcome to condense the above to update
MaintNotes in my 'todo' branch.
[Footnote]
*1* Merges that appear early in the "--first-parent master..pu"
carry the topic summary I have in the draft of "What's cooking"
report as its merge message.
*2* Sometimes a maint-worthy patch series is done in such a way that
it only applies to 'next'. I try to tweak such a patch series
to apply 'maint' and either use the result of my tweak, or ask
the submitter to base it on 'maint', depending on how confident
I am with the tweaked result.
*3* When I do this and any other "merge" of a topic branch into an
integration branch, the topic description in the draft "What's
cooking" report is used as the merge log message, if one already
exists.
*4* Readers would notice that 'jch' and 'pu' are not decendants of
'next'; this is very much deliberate, and it has helped me to
spot mismerges of tricky topics when they graduate to 'master' a
few times.
*5* IOW, I go back to the step "around 14:00-15:00".
*6* This is why the tip of 'pu' sometimes may not even build. IOW,
a broken 'pu' is an invitation for help, not complaint.
*7* Remember *3* above and also the fact that "What's cooking" draft
is updated only after everything is merged to 'pu'; there is a
small chicken and egg problem here and redoing 'jch/pu' will fix
it.
*8* Broken-out individual topics are sent to github.com/gitster/git
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2017, #02; Mon, 6)
From: Junio C Hamano @ 2017-02-09 19:18 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git
In-Reply-To: <2f67fc21-92f9-a03e-1b09-a237af6dbc46@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> On 02/06/2017 11:34 PM, Junio C Hamano wrote:
>> [...]
>> --------------------------------------------------
>> [Stalled]
>> [...]
>> * mh/ref-remove-empty-directory (2017-01-07) 23 commits
>>
>> Deletion of a branch "foo/bar" could remove .git/refs/heads/foo
>> once there no longer is any other branch whose name begins with
>> "foo/", but we didn't do so so far. Now we do.
>>
>> Expecting a reroll.
>> cf. <5051c78e-51f9-becd-e1a6-9c0b781d6912@alum.mit.edu>
>
> I think you missed v4 of this patch series [1], which is the re-roll
> that you were waiting for. And I missed that you missed it...
>
> Michael
>
> [1] http://public-inbox.org/git/cover.1483719289.git.mhagger@alum.mit.edu/
Great. Thanks.
^ permalink raw reply
* Re: [PATCH v3 00/27] Revamp the attribute system; another round
From: Junio C Hamano @ 2017-02-09 19:31 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, pclouds
In-Reply-To: <20170209171804.GA61274@google.com>
Brandon Williams <bmwill@google.com> writes:
> At least v3 gets the attribute system to a state where further
> improvements should be relatively easy to make. And now as long as each
> thread has a unique attr_check structure, multiple callers can exist
> inside the attribute system at the same time. There is still more work
> to be done on it though. Still my biggest complaint is the "direction"
> aspect of the system. I would love to also eliminate that as global
> state at some point though I'm not sure how at this point.
We are in agreement 100% ;-) The "direction" was the last thorn I
was fighting with (without successfully coming up with a usable
solution) when I stopped working on my original series before Stefan
took it over.
^ permalink raw reply
* Re: [PATCH 1/5] refs: store submodule ref stores in a hashmap
From: Jeff King @ 2017-02-09 19:32 UTC (permalink / raw)
To: Michael Haggerty
Cc: Stefan Beller, Junio C Hamano,
Nguyễn Thái Ngọc Duy, Johannes Schindelin,
David Turner, git@vger.kernel.org
In-Reply-To: <4a21dba7-76ef-6aec-b326-c1046f3daad2@alum.mit.edu>
On Thu, Feb 09, 2017 at 06:40:29PM +0100, Michael Haggerty wrote:
> On 02/09/2017 05:58 PM, Stefan Beller wrote:
> >> @@ -1402,17 +1435,17 @@ struct ref_store *ref_store_init(const char *submodule)
> >>
> >> struct ref_store *lookup_ref_store(const char *submodule)
> >> {
> >
> >> + if (!submodule_ref_stores.tablesize)
> >> + hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 20);
> >
> >
> > So we can lookup a submodule even before we initialized the subsystem?
> > Does that actually happen? (It sounds like a bug to me.)
> >
> > Instead of initializing, you could return NULL directly here.
> [...]
> I suppose this code path could be changed to return NULL without
> initializing the hashmap, but the hashmap will be initialized a moment
> later by ref_store_init(), so I don't see much difference either way.
I faced a similar issue when adding the oidset API recently:
http://public-inbox.org/git/20170208205307.uvgj3saf3cyrvtan@sigill.intra.peff.net/
I came to the same conclusion that it doesn't matter much in practice. A
nice thing about "return NULL" is that you do not have to duplicate the
initialization which happens on the "write" side (so if you ever changed
submodule_hash_cmp, for example, it needs changed in both places).
I also used the "cmpfn" member to check whether the table had been
initialized, which matches existing uses elsewhere. But I do notice that
the documentation explicitly says "tablesize" is good for this purpose,
so it's probably a better choice.
-Peff
^ permalink raw reply
* Re: Bug with fixup and autosquash
From: Junio C Hamano @ 2017-02-09 19:46 UTC (permalink / raw)
To: Michael J Gruber
Cc: Ashutosh Bapat, git, Johannes Schindelin, Michael Haggerty,
Matthieu Moy
In-Reply-To: <07eb2367-9509-afb0-2494-f02a44304bc4@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
> Junio C Hamano venit, vidit, dixit 08.02.2017 23:55:
>
>> Let's hear from some of those (Cc'ed) who were involved in an
>> earlier --autosquash thread.
>>
>> https://public-inbox.org/git/cover.1259934977.git.mhagger@alum.mit.edu/
>>
>>
>> [Footnote]
>>
>> *1* "rebase -i --autosquash" does understand "fixup! yyyyyy", so if
>> you are willing to accept the consequence of not being able to
>> rebase twice, you could instead do
>>
>> $ git commit -m "fixup! yyyyyy"
>>
>> I would think.
>
> Doesn't this indicate that rebase is fine as is?
Not really, unless you ignore "if you are willing to accept" part,
which actually is a big downside. And --fixup-fixed will make it
worse, unfortunately.
> - teach "git commit --fixup=<rev>" to check for duplicates (same prefix,
> maybe only in "recent" history) and make it issue a warning, say:
This is a very good idea worth pursuing, and (I didn't think things
through, though) we may even be able to bound "recent" without
heuristics---scanning between <rev> and the tip for duplicates might
be sufficient.
> Additionally, we could teach commit --fixup-fixed to commit -m "fixup!
> <sha1> <prefix>" so that we have both uniqueness and verbosity in the
> rebase-i-editor. This would allow "rebase -i" to fall back to the old
> mode when "<sha1>" is not in the range it operates on.
This is also a possibility, but it needs cooperation between both
"commit" and "rebase -i".
I personally do not think rewriting "fixup! yyyyyy" on the title
during rebase is worth doing, but that is not because I have a
concrete reason against it but it just sounds like too much magic to
my gut feeling. Perhaps it can be done reliably with minimal change
to the code. I dunno.
Thanks.
^ permalink raw reply
* Re: [PATCH 0/5] Store submodules in a hash, not a linked list
From: Junio C Hamano @ 2017-02-09 19:48 UTC (permalink / raw)
To: Michael Haggerty
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller,
Johannes Schindelin, David Turner, git
In-Reply-To: <cover.1486629195.git.mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> I have mentioned this patch series on the mailing list a couple of
> time [1,2] but haven't submitted it before. I just rebased it to
> current master. It is available from my Git fork [3] as branch
> "submodule-hash".
>
> The first point of this patch series is to optimize submodule
> `ref_store` lookup by storing the `ref_store`s in a hashmap rather
> than a linked list. But a more interesting second point is to weaken
> the 1:1 relationship between submodules and `ref_stores` a little bit
> more.
>
> A `files_ref_store` would be perfectly happy to represent, say, the
> references *physically* stored in a linked worktree (e.g., `HEAD`,
> `refs/bisect/*`, etc) even though that is not the complete collection
> of refs that are *logically* visible from that worktree (which
> includes references from the main repository, too). But the old code
> was confusing the two things by storing "submodule" in every
> `ref_store` instance.
>
> So push the submodule attribute down to the `files_ref_store` class
> (but continue to let the `ref_store`s be looked up by submodule).
>
> The last commit is relatively orthogonal to the others; it simplifies
> read_loose_refs() by calling resolve_ref_recursively() directly using
> the `ref_store` instance that it already has in hand, rather than
> indirectly via the public wrappers.
>
> Michael
>
> [1] http://public-inbox.org/git/341999fc-4496-b974-c117-c18a2fca1358@alum.mit.edu/
> [2] http://public-inbox.org/git/37fe2024-0378-a974-a28d-18a89d3e2312@alum.mit.edu/
> [3] https://github.com/mhagger/git
>
> Michael Haggerty (5):
> refs: store submodule ref stores in a hashmap
> refs: push the submodule attribute down
> register_ref_store(): new function
> files_ref_store::submodule: use NULL for the main repository
> read_loose_refs(): read refs using resolve_ref_recursively()
Thanks. Will queue on mh/submodule-hash forked from 'maint'.
^ permalink raw reply
* Re: [PATCH 0/5] Store submodules in a hash, not a linked list
From: Jeff King @ 2017-02-09 19:58 UTC (permalink / raw)
To: Michael Haggerty
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Stefan Beller, Johannes Schindelin, David Turner, git
In-Reply-To: <cover.1486629195.git.mhagger@alum.mit.edu>
On Thu, Feb 09, 2017 at 02:26:57PM +0100, Michael Haggerty wrote:
> I have mentioned this patch series on the mailing list a couple of
> time [1,2] but haven't submitted it before. I just rebased it to
> current master. It is available from my Git fork [3] as branch
> "submodule-hash".
>
> The first point of this patch series is to optimize submodule
> `ref_store` lookup by storing the `ref_store`s in a hashmap rather
> than a linked list. But a more interesting second point is to weaken
> the 1:1 relationship between submodules and `ref_stores` a little bit
> more.
Sounds good. I remember this had been discussed before due to
performance issues with resolve_gitlink_ref(), and we took a different
route (not populating non-submodule entries). I think it's nice to have
both optimizations, though, as they hit different use cases.
> A `files_ref_store` would be perfectly happy to represent, say, the
> references *physically* stored in a linked worktree (e.g., `HEAD`,
> `refs/bisect/*`, etc) even though that is not the complete collection
> of refs that are *logically* visible from that worktree (which
> includes references from the main repository, too). But the old code
> was confusing the two things by storing "submodule" in every
> `ref_store` instance.
>
> So push the submodule attribute down to the `files_ref_store` class
> (but continue to let the `ref_store`s be looked up by submodule).
I'm not sure I understand all of the ramifications here. It _sounds_ like
pushing this down into the files-backend code would make it harder to
have mixed ref-backends for different submodules. Or is this just
pushing down an implementation detail of the files backend, and future
code is free to have as many different ref_stores as it likes?
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2017, #02; Mon, 6)
From: Junio C Hamano @ 2017-02-09 19:24 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git
In-Reply-To: <2f67fc21-92f9-a03e-1b09-a237af6dbc46@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> On 02/06/2017 11:34 PM, Junio C Hamano wrote:
>> [...]
>> --------------------------------------------------
>> [Stalled]
>> [...]
>> * mh/ref-remove-empty-directory (2017-01-07) 23 commits
>> - files_transaction_commit(): clean up empty directories
>> - try_remove_empty_parents(): teach to remove parents of reflogs, too
>> - try_remove_empty_parents(): don't trash argument contents
>> - try_remove_empty_parents(): rename parameter "name" -> "refname"
>> - delete_ref_loose(): inline function
>> - delete_ref_loose(): derive loose reference path from lock
>> - log_ref_write_1(): inline function
>> - log_ref_setup(): manage the name of the reflog file internally
>> - log_ref_write_1(): don't depend on logfile argument
>> - log_ref_setup(): pass the open file descriptor back to the caller
>> - log_ref_setup(): improve robustness against races
>> - log_ref_setup(): separate code for create vs non-create
>> - log_ref_write(): inline function
>> - rename_tmp_log(): improve error reporting
>> - rename_tmp_log(): use raceproof_create_file()
>> - lock_ref_sha1_basic(): use raceproof_create_file()
>> - lock_ref_sha1_basic(): inline constant
>> - raceproof_create_file(): new function
>> - safe_create_leading_directories(): set errno on SCLD_EXISTS
>> - safe_create_leading_directories_const(): preserve errno
>> - t5505: use "for-each-ref" to test for the non-existence of references
>> - refname_is_safe(): correct docstring
>> - files_rename_ref(): tidy up whitespace
>>
>> Deletion of a branch "foo/bar" could remove .git/refs/heads/foo
>> once there no longer is any other branch whose name begins with
>> "foo/", but we didn't do so so far. Now we do.
>>
>> Expecting a reroll.
>> cf. <5051c78e-51f9-becd-e1a6-9c0b781d6912@alum.mit.edu>
>
> I think you missed v4 of this patch series [1], which is the re-roll
> that you were waiting for. And I missed that you missed it...
>
> Michael
>
> [1] http://public-inbox.org/git/cover.1483719289.git.mhagger@alum.mit.edu/
Actually it was worse than that. What the above lists *is* v4; I
just failed to update "Expecting a reroll" note when I updated the
topic with your rerolled patches, and left it there trusting the
now-stale note of mine.
Sorry, and a HUGE thanks for noticing the mistake.
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2017, #02; Mon, 6)
From: Johannes Schindelin @ 2017-02-09 20:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqfujns2li.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 9 Feb 2017, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > (And that would have to be handled at a different point, as I had
> > pointed out, so that suggested preparation would most likely not help
> > at all.)
>
> I did not think "that would have to be handled at a different point"
> is correct at all, if by "a point" you meant "a location in the
> code" [*1*].
Yes, I mean the location in the code.
But since you keep insisting that you are right and I am wrong, and even
go so far as calling your patch reverting my refactoring a hot-fix, why
don't you just go ahead and merge the result over my objections?
Ciao,
Johannes
^ 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