* [PATCH] git-p4: add p4 shelf support
From: Nuno Subtil @ 2016-12-06 2:02 UTC (permalink / raw)
To: git
Extends the submit command to support shelving a commit instead of
submitting it to p4 (similar to --prepare-p4-only).
Signed-off-by: Nuno Subtil <subtil@gmail.com>
---
git-p4.py | 36 ++++++++++++++++++++++++++++++------
1 file changed, 30 insertions(+), 6 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index fd5ca52..3c4be22 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1286,6 +1286,8 @@ def __init__(self):
optparse.make_option("--export-labels", dest="exportLabels", action="store_true"),
optparse.make_option("--dry-run", "-n", dest="dry_run", action="store_true"),
optparse.make_option("--prepare-p4-only", dest="prepare_p4_only", action="store_true"),
+ optparse.make_option("--shelve-only", dest="shelve_only", action="store_true", help="Create P4 shelf for first change that would be submitted (using a new CL)"),
+ optparse.make_option("--shelve-cl", dest="shelve_cl", help="Replace shelf under existing CL number (previously shelved files will be deleted)"),
optparse.make_option("--conflict", dest="conflict_behavior",
choices=self.conflict_behavior_choices),
optparse.make_option("--branch", dest="branch"),
@@ -1297,6 +1299,8 @@ def __init__(self):
self.preserveUser = gitConfigBool("git-p4.preserveUser")
self.dry_run = False
self.prepare_p4_only = False
+ self.shelve_only = False
+ self.shelve_cl = None
self.conflict_behavior = None
self.isWindows = (platform.system() == "Windows")
self.exportLabels = False
@@ -1496,6 +1500,12 @@ def prepareSubmitTemplate(self):
else:
inFilesSection = False
else:
+ if self.shelve_only and self.shelve_cl:
+ if line.startswith("Change:"):
+ line = "Change: %s\n" % self.shelve_cl
+ if line.startswith("Status:"):
+ line = "Status: pending\n"
+
if line.startswith("Files:"):
inFilesSection = True
@@ -1785,7 +1795,11 @@ def applyCommit(self, id):
if self.isWindows:
message = message.replace("\r\n", "\n")
submitTemplate = message[:message.index(separatorLine)]
- p4_write_pipe(['submit', '-i'], submitTemplate)
+
+ if self.shelve_only:
+ p4_write_pipe(['shelve', '-i', '-r'], submitTemplate)
+ else:
+ p4_write_pipe(['submit', '-i'], submitTemplate)
if self.preserveUser:
if p4User:
@@ -1799,12 +1813,17 @@ def applyCommit(self, id):
# new file. This leaves it writable, which confuses p4.
for f in pureRenameCopy:
p4_sync(f, "-f")
- submitted = True
+
+ if not self.shelve_only:
+ submitted = True
finally:
# skip this patch
if not submitted:
- print "Submission cancelled, undoing p4 changes."
+ if not self.shelve_only:
+ print "Submission cancelled, undoing p4 changes."
+ else:
+ print "Change shelved, undoing p4 changes."
for f in editedFiles:
p4_revert(f)
for f in filesToAdd:
@@ -2034,9 +2053,13 @@ def run(self, args):
if ok:
applied.append(commit)
else:
- if self.prepare_p4_only and i < last:
- print "Processing only the first commit due to option" \
- " --prepare-p4-only"
+ if (self.prepare_p4_only or self.shelve_only) and i < last:
+ if self.prepare_p4_only:
+ print "Processing only the first commit due to option" \
+ " --prepare-p4-only"
+ else:
+ print "Processing only the first commit due to option" \
+ " --shelve-only"
break
if i < last:
quit = False
@@ -3638,6 +3661,7 @@ def printUsage(commands):
"debug" : P4Debug,
"submit" : P4Submit,
"commit" : P4Submit,
+ "shelve" : P4Submit,
"sync" : P4Sync,
"rebase" : P4Rebase,
"clone" : P4Clone,
--
https://github.com/git/git/pull/309
^ permalink raw reply related
* [PATCH v3] diff: handle --no-abbrev in no-index case
From: Jack Bates @ 2016-12-06 1:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Jack Bates
In-Reply-To: <20161205065823.c7qw6xtc2hqk3xgu@sigill.intra.peff.net>
There are two different places where the --no-abbrev option is parsed,
and two different places where SHA-1s are abbreviated. We normally parse
--no-abbrev with setup_revisions(), but in the no-index case, "git diff"
calls diff_opt_parse() directly, and diff_opt_parse() didn't handle
--no-abbrev until now. (It did handle --abbrev, however.) We normally
abbreviate SHA-1s with find_unique_abbrev(), but commit 4f03666 ("diff:
handle sha1 abbreviations outside of repository, 2016-10-20) recently
introduced a special case when you run "git diff" outside of a
repository.
setup_revisions() does also call diff_opt_parse(), but not for --abbrev
or --no-abbrev, which it handles itself. setup_revisions() sets
rev_info->abbrev, and later copies that to diff_options->abbrev. It
handles --no-abbrev by setting abbrev to zero. (This change doesn't
touch that.)
Setting abbrev to zero was broken in the outside-of-a-repository special
case, which until now resulted in a truly zero-length SHA-1, rather than
taking zero to mean do not abbreviate. The only way to trigger this bug,
however, was by running "git diff --raw" without either the --abbrev or
--no-abbrev options, because 1) without --raw it doesn't respect abbrev
(which is bizarre, but has been that way forever), 2) we silently clamp
--abbrev=0 to MINIMUM_ABBREV, and 3) --no-abbrev wasn't handled until
now.
The outside-of-a-repository case is one of three no-index cases. The
other two are when one of the files you're comparing is outside of the
repository you're in, and the --no-index option.
Signed-off-by: Jack Bates <jack@nottheoilrig.com>
---
diff.c | 6 +++++-
t/t4013-diff-various.sh | 7 +++++++
t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir | 3 +++
t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir | 3 +++
t/t4013/diff.diff_--no-index_--raw_dir2_dir | 3 +++
t/t4013/diff.diff_--raw_--abbrev=4_initial | 6 ++++++
t/t4013/diff.diff_--raw_--no-abbrev_initial | 6 ++++++
t/t4013/diff.diff_--raw_initial | 6 ++++++
8 files changed, 39 insertions(+), 1 deletion(-)
create mode 100644 t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
create mode 100644 t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
create mode 100644 t/t4013/diff.diff_--no-index_--raw_dir2_dir
create mode 100644 t/t4013/diff.diff_--raw_--abbrev=4_initial
create mode 100644 t/t4013/diff.diff_--raw_--no-abbrev_initial
create mode 100644 t/t4013/diff.diff_--raw_initial
diff --git a/diff.c b/diff.c
index ec87283..84dba60 100644
--- a/diff.c
+++ b/diff.c
@@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)
abbrev = FALLBACK_DEFAULT_ABBREV;
if (abbrev > GIT_SHA1_HEXSZ)
die("BUG: oid abbreviation out of range: %d", abbrev);
- hex[abbrev] = '\0';
+ if (abbrev)
+ hex[abbrev] = '\0';
return hex;
}
}
@@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)
options->file = stdout;
+ options->abbrev = DEFAULT_ABBREV;
options->line_termination = '\n';
options->break_opt = -1;
options->rename_limit = -1;
@@ -4024,6 +4026,8 @@ int diff_opt_parse(struct diff_options *options,
offending, optarg);
return argcount;
}
+ else if (!strcmp(arg, "--no-abbrev"))
+ options->abbrev = 0;
else if (!strcmp(arg, "--abbrev"))
options->abbrev = DEFAULT_ABBREV;
else if (skip_prefix(arg, "--abbrev=", &arg)) {
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 566817e..d7b71a0 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -311,6 +311,13 @@ diff --line-prefix=abc master master^ side
diff --dirstat master~1 master~2
diff --dirstat initial rearrange
diff --dirstat-by-file initial rearrange
+# --abbrev and --no-abbrev outside of repository
+diff --raw initial
+diff --raw --abbrev=4 initial
+diff --raw --no-abbrev initial
+diff --no-index --raw dir2 dir
+diff --no-index --raw --abbrev=4 dir2 dir
+diff --no-index --raw --no-abbrev dir2 dir
EOF
test_expect_success 'log -S requires an argument' '
diff --git a/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
new file mode 100644
index 0000000..a71b38a
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --abbrev=4 dir2 dir
+:000000 100644 0000... 0000... A dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
new file mode 100644
index 0000000..e0f0097
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --no-abbrev dir2 dir
+:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
new file mode 100644
index 0000000..3cb4ee7
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw dir2 dir
+:000000 100644 0000000... 0000000... A dir/sub
+$
diff --git a/t/t4013/diff.diff_--raw_--abbrev=4_initial b/t/t4013/diff.diff_--raw_--abbrev=4_initial
new file mode 100644
index 0000000..c3641db
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--abbrev=4_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --abbrev=4 initial
+:100644 100644 35d2... 9929... M dir/sub
+:100644 100644 01e7... 10a8... M file0
+:000000 100644 0000... b1e6... A file1
+:100644 000000 01e7... 0000... D file2
+$
diff --git a/t/t4013/diff.diff_--raw_--no-abbrev_initial b/t/t4013/diff.diff_--raw_--no-abbrev_initial
new file mode 100644
index 0000000..c87a125
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--no-abbrev_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --no-abbrev initial
+:100644 100644 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 M dir/sub
+:100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M file0
+:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A file1
+:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D file2
+$
diff --git a/t/t4013/diff.diff_--raw_initial b/t/t4013/diff.diff_--raw_initial
new file mode 100644
index 0000000..a3e9780
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_initial
@@ -0,0 +1,6 @@
+$ git diff --raw initial
+:100644 100644 35d242b... 992913c... M dir/sub
+:100644 100644 01e79c3... 10a8a9f... M file0
+:000000 100644 0000000... b1e6722... A file1
+:100644 000000 01e79c3... 0000000... D file2
+$
--
2.10.2
^ permalink raw reply related
* Re: [RFC PATCHv2 08/17] update submodules: add depopulate_submodule
From: Stefan Beller @ 2016-12-06 0:18 UTC (permalink / raw)
To: David Turner
Cc: bmwill@google.com, git@vger.kernel.org,
sandals@crustytoothpaste.net, hvoigt@hvoigt.net,
gitster@pobox.com
In-Reply-To: <832dcc3eec0d4237a1e2766e8df690ee@exmbdft7.ad.twosigma.com>
On Mon, Dec 5, 2016 at 3:37 PM, David Turner <David.Turner@twosigma.com> wrote:
>> -----Original Message-----
>> From: Stefan Beller [mailto:sbeller@google.com]
>> Sent: Friday, December 02, 2016 7:30 PM
>> To: bmwill@google.com; David Turner
>> Cc: git@vger.kernel.org; sandals@crustytoothpaste.net; hvoigt@hvoigt.net;
>> gitster@pobox.com; Stefan Beller
>> Subject: [RFC PATCHv2 08/17] update submodules: add depopulate_submodule
>>
>> Implement the functionality needed to enable work tree manipulating
>> commands so that a deleted submodule should not only affect the index
>> (leaving all the files of the submodule in the work tree) but also to
>> remove the work tree of the superproject (including any untracked files).
>
> "including any untracked files" bothers me, I think. Checkout is not usually willing to overwrite untracked files; it seems odd to me that it would be willing to do so in the submodule case. I would be OK if they were both untracked and gitignored, I think.
I agree on being bothered, this is one of the things I thought how to solve.
See the test in "checkout: recurse into submodules if asked to", which
tests for untracked files and is still marked as a failure.
I think to address that issue, I'll add a flag to ok_to_remove_submodule
which let's you specify which files you care about and which you don't.
>> + warning(_("Cannot remove submodule '%s'\n"
>> + "because it (or one of its nested submodules)
>> has a git \n"
>> + "directory in the working tree, which could not
>> be embedded\n"
>> + "the superprojects git directory
>> automatically."), path);
>
> What if instead it couldn't run the command because you're out of file descriptors or pids or memory or something?
>
> I think this message should be in submodule--helper --embed-git-dirs instead, and we should just pass it through here. Or, perhaps, instead of shelling out here, we should just call the functions directly?
heh, good point. Will call the function directly.
>
^ permalink raw reply
* Re: [RFC PATCHv2 09/17] update submodules: add scheduling to update submodules
From: Stefan Beller @ 2016-12-05 23:54 UTC (permalink / raw)
To: David Turner
Cc: bmwill@google.com, git@vger.kernel.org,
sandals@crustytoothpaste.net, hvoigt@hvoigt.net,
gitster@pobox.com
In-Reply-To: <f19844d15ab4424b8c056cd13837d233@exmbdft7.ad.twosigma.com>
On Mon, Dec 5, 2016 at 3:37 PM, David Turner <David.Turner@twosigma.com> wrote:
> This patch confuses me -- see below.
>
>> -----Original Message-----
>> From: Stefan Beller [mailto:sbeller@google.com]
>> Sent: Friday, December 02, 2016 7:30 PM
>> To: bmwill@google.com; David Turner
>> Cc: git@vger.kernel.org; sandals@crustytoothpaste.net; hvoigt@hvoigt.net;
>> gitster@pobox.com; Stefan Beller
>> Subject: [RFC PATCHv2 09/17] update submodules: add scheduling to update
>> submodules
> [snip]
>> +static int update_submodule(const char *path, const struct object_id
>> *oid,
>> + int force, int is_new)
>> +{
>> + const char *git_dir;
>> + struct child_process cp = CHILD_PROCESS_INIT;
>> + const struct submodule *sub = submodule_from_path(null_sha1, path);
>> +
>> + if (!sub || !sub->name)
>> + return -1;
>> +
>> + git_dir = resolve_gitdir(git_common_path("modules/%s", sub->name));
>> +
>> + if (!git_dir)
>> + return -1;
>> +
>> + if (is_new)
>> + connect_work_tree_and_git_dir(path, git_dir);
>> +
>> + /* update index via `read-tree --reset sha1` */
>> + argv_array_pushl(&cp.args, "read-tree",
>> + force ? "--reset" : "-m",
>> + "-u", sha1_to_hex(oid->hash), NULL);
>> + prepare_submodule_repo_env(&cp.env_array);
>> + cp.git_cmd = 1;
>> + cp.no_stdin = 1;
>> + cp.dir = path;
>> + if (run_command(&cp)) {
>> + warning(_("reading the index in submodule '%s' failed"),
>> path);
>
> The error is not (usually) in "reading the index" -- it's "updating the index" (or the working tree)
>
>> + child_process_clear(&cp);
>> + return -1;
>> + }
>> +
>> + /* write index to working dir */
>> + child_process_clear(&cp);
>> + child_process_init(&cp);
>> + argv_array_pushl(&cp.args, "checkout-index", "-a", NULL);
>
> I'm confused -- doesn't read-tree -u already do this? And if not, shouldn't we back out the result of the read-tree, to leave the submodule as it was?
>
>> + cp.git_cmd = 1;
>> + cp.no_stdin = 1;
>> + cp.dir = path;
>> + if (force)
>> + argv_array_push(&cp.args, "-f");
>> +
>> + if (run_command(&cp)) {
>> + warning(_("populating the working directory in submodule '%s'
>> failed"), path);
>> + child_process_clear(&cp);
>> + return -1;
>> + }
>> +
>> + /* get the HEAD right */
>> + child_process_clear(&cp);
>> + child_process_init(&cp);
>> + argv_array_pushl(&cp.args, "checkout", "--recurse-submodules",
>> NULL);
>
>
> Why are we running checkout on the submodule when we've already done most of the checkout? The only thing left is to set HEAD and recurse, right? I must be missing something.
>
Yes this is only used to set the HEAD correctly and then recurse down.
I tried to remove the first 2 calls to ch8ild processes at one point in time,
which did not work out. I should have written in the commit message why
that was a problem. So I'll redo that just to see the problem and improve
the commit message.
^ permalink raw reply
* RE: [RFC PATCHv2 08/17] update submodules: add depopulate_submodule
From: David Turner @ 2016-12-05 23:37 UTC (permalink / raw)
To: 'Stefan Beller', bmwill@google.com
Cc: git@vger.kernel.org, sandals@crustytoothpaste.net,
hvoigt@hvoigt.net, gitster@pobox.com
In-Reply-To: <20161203003022.29797-9-sbeller@google.com>
> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
> Sent: Friday, December 02, 2016 7:30 PM
> To: bmwill@google.com; David Turner
> Cc: git@vger.kernel.org; sandals@crustytoothpaste.net; hvoigt@hvoigt.net;
> gitster@pobox.com; Stefan Beller
> Subject: [RFC PATCHv2 08/17] update submodules: add depopulate_submodule
>
> Implement the functionality needed to enable work tree manipulating
> commands so that a deleted submodule should not only affect the index
> (leaving all the files of the submodule in the work tree) but also to
> remove the work tree of the superproject (including any untracked files).
"including any untracked files" bothers me, I think. Checkout is not usually willing to overwrite untracked files; it seems odd to me that it would be willing to do so in the submodule case. I would be OK if they were both untracked and gitignored, I think.
> To do so, we need an equivalent of "rm -rf", which is already found in
> entry.c, so expose that and for clarity add a suffix "_or_dir" to it.
>
> That will only work properly when the submodule uses a gitfile instead of
> a .git directory and no untracked files are present. Otherwise the removal
> will fail with a warning (which is just what happened until now).
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> cache.h | 2 ++
> entry.c | 5 +++++
> submodule.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> submodule.h | 1 +
> 4 files changed, 54 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index a50a61a197..b645ca2f9a 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -2018,4 +2018,6 @@ void sleep_millisec(int millisec);
> */
> void safe_create_dir(const char *dir, int share);
>
> +extern void remove_directory_or_die(struct strbuf *path);
> +
> #endif /* CACHE_H */
> diff --git a/entry.c b/entry.c
> index c6eea240b6..02c4ac9f22 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -73,6 +73,11 @@ static void remove_subtree(struct strbuf *path)
> die_errno("cannot rmdir '%s'", path->buf); }
>
> +void remove_directory_or_die(struct strbuf *path) {
> + remove_subtree(path);
> +}
> +
> static int create_file(const char *path, unsigned int mode) {
> mode = (mode & 0100) ? 0777 : 0666;
> diff --git a/submodule.c b/submodule.c
> index 62e9ef3872..7bb64d6c69 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -324,6 +324,52 @@ void prepare_submodule_repo_env(struct argv_array
> *out)
> argv_array_push(out, "GIT_DIR=.git");
> }
>
> +int depopulate_submodule(const char *path) {
> + int ret = 0;
> + struct strbuf pathbuf = STRBUF_INIT;
> + char *dot_git = xstrfmt("%s/.git", path);
> +
> + /* Is it populated? */
> + if (!resolve_gitdir(dot_git))
> + goto out;
> +
> + /* Does it have a .git directory? */
> + if (!submodule_uses_gitfile(path)) {
> + struct child_process cp = CHILD_PROCESS_INIT;
> +
> + prepare_submodule_repo_env(&cp.env_array);
> + argv_array_pushl(&cp.args, "submodule--helper",
> + "embed-git-dirs", path, NULL);
> + cp.git_cmd = 1;
> + if (run_command(&cp)) {
> + warning(_("Cannot remove submodule '%s'\n"
> + "because it (or one of its nested submodules)
> has a git \n"
> + "directory in the working tree, which could not
> be embedded\n"
> + "the superprojects git directory
> automatically."), path);
What if instead it couldn't run the command because you're out of file descriptors or pids or memory or something?
I think this message should be in submodule--helper --embed-git-dirs instead, and we should just pass it through here. Or, perhaps, instead of shelling out here, we should just call the functions directly?
> + ret = -1;
> + goto out;
> + }
> +
> + if (!submodule_uses_gitfile(path)) {
> + /*
> + * We should be using a gitfile by now, let's double
Comma splice.
> + * check as loosing the git dir would be fatal.
s/loosing/losing/
> + */
> + die("BUG: \"git submodule--helper embed git-dirs '%s'\"
> "
> + "did not embed the git-dirs recursively for '%s'",
> + path, path);
> + }
> + }
> +
> + strbuf_addstr(&pathbuf, path);
> + remove_directory_or_die(&pathbuf);
> +out:
> + strbuf_release(&pathbuf);
> + free(dot_git);
> + return ret;
> +}
> +
> /* Helper function to display the submodule header line prior to the full
> * summary output. If it can locate the submodule objects directory it
> will
> * attempt to lookup both the left and right commits and put them into
> the diff --git a/submodule.h b/submodule.h index 7d890e0464..d8bb1d4baf
> 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -63,6 +63,7 @@ extern void set_config_update_recurse_submodules(int
> value);
> */
> extern int submodule_is_interesting(const char *path); extern int
> submodules_interesting_for_update(void);
> +extern int depopulate_submodule(const char *path);
> extern void check_for_new_submodule_commits(unsigned char new_sha1[20]);
> extern int fetch_populated_submodules(const struct argv_array *options,
> const char *prefix, int command_line_option,
> --
> 2.11.0.rc2.28.g2673dad
^ permalink raw reply
* RE: [RFC PATCHv2 14/17] submodule: teach unpack_trees() to update submodules
From: David Turner @ 2016-12-05 23:37 UTC (permalink / raw)
To: 'Stefan Beller', bmwill@google.com
Cc: git@vger.kernel.org, sandals@crustytoothpaste.net,
hvoigt@hvoigt.net, gitster@pobox.com
In-Reply-To: <20161203003022.29797-15-sbeller@google.com>
> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
> Sent: Friday, December 02, 2016 7:30 PM
> To: bmwill@google.com; David Turner
> Cc: git@vger.kernel.org; sandals@crustytoothpaste.net; hvoigt@hvoigt.net;
> gitster@pobox.com; Stefan Beller
> Subject: [RFC PATCHv2 14/17] submodule: teach unpack_trees() to update
> submodules
[snip]
> if (!lstat(ce->name, &st)) {
> - int flags =
> CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;
> - unsigned changed = ie_match_stat(o->src_index, ce, &st,
> flags);
> - if (!changed)
> - return 0;
> - /*
> - * NEEDSWORK: the current default policy is to allow
> - * submodule to be out of sync wrt the superproject
> - * index. This needs to be tightened later for
> - * submodules that are marked to be automatically
> - * checked out.
> - */
> - if (S_ISGITLINK(ce->ce_mode))
> - return 0;
> + if (S_ISGITLINK(ce->ce_mode)) {
> + if (!submodule_is_interesting(ce->name))
> + return 0;
> + if (ce_stage(ce) ? is_submodule_checkout_safe(ce->name,
> &ce->oid)
> + : !is_submodule_modified(ce->name, 1))
This logic is too convoluted. Do a nested if or something.
> + return 0;
> + } else {
> + int flags = CE_MATCH_IGNORE_VALID |
> CE_MATCH_IGNORE_SKIP_WORKTREE;
> + if (!ie_match_stat(o->src_index, ce, &st, flags))
> + return 0;
Nit: I liked the old temp var "changed" better -- it made it clear what
ie_match_stat is checking.
> + }
> errno = 0;
> }
> if (errno == ENOENT)
> @@ -1355,6 +1359,38 @@ static int verify_uptodate_sparse(const struct
> cache_entry *ce,
> return verify_uptodate_1(ce, o, ERROR_SPARSE_NOT_UPTODATE_FILE); }
>
> +/*
> + * When a submodule gets turned into an unmerged entry, we want it to
> +be
> + * up-to-date regarding the merge changes.
> + */
> +static int verify_uptodate_submodule(const struct cache_entry *old,
> + const struct cache_entry *new,
> + struct unpack_trees_options *o) {
> + struct stat st;
> +
> + if (o->index_only ||
> + (!((old->ce_flags & CE_VALID) || ce_skip_worktree(old)) &&
> + (o->reset || ce_uptodate(old))))
> + return 0;
> +
> + if (!submodule_is_interesting(new->name))
> + return 0;
> +
> + if (lstat(old->name, &st)) {
We never actually use st here. Should we? If not, is this really the right error message? And should we use access() instead of lstat?
> + if (errno == ENOENT)
> + return 0;
> + return o->gently ? -1 :
> + add_rejected_path(o, ERROR_NOT_UPTODATE_SUBMODULE,
> + old->name);
> + }
> +
^ permalink raw reply
* RE: [RFC PATCHv2 09/17] update submodules: add scheduling to update submodules
From: David Turner @ 2016-12-05 23:37 UTC (permalink / raw)
To: 'Stefan Beller', bmwill@google.com
Cc: git@vger.kernel.org, sandals@crustytoothpaste.net,
hvoigt@hvoigt.net, gitster@pobox.com
In-Reply-To: <20161203003022.29797-10-sbeller@google.com>
This patch confuses me -- see below.
> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
> Sent: Friday, December 02, 2016 7:30 PM
> To: bmwill@google.com; David Turner
> Cc: git@vger.kernel.org; sandals@crustytoothpaste.net; hvoigt@hvoigt.net;
> gitster@pobox.com; Stefan Beller
> Subject: [RFC PATCHv2 09/17] update submodules: add scheduling to update
> submodules
[snip]
> +static int update_submodule(const char *path, const struct object_id
> *oid,
> + int force, int is_new)
> +{
> + const char *git_dir;
> + struct child_process cp = CHILD_PROCESS_INIT;
> + const struct submodule *sub = submodule_from_path(null_sha1, path);
> +
> + if (!sub || !sub->name)
> + return -1;
> +
> + git_dir = resolve_gitdir(git_common_path("modules/%s", sub->name));
> +
> + if (!git_dir)
> + return -1;
> +
> + if (is_new)
> + connect_work_tree_and_git_dir(path, git_dir);
> +
> + /* update index via `read-tree --reset sha1` */
> + argv_array_pushl(&cp.args, "read-tree",
> + force ? "--reset" : "-m",
> + "-u", sha1_to_hex(oid->hash), NULL);
> + prepare_submodule_repo_env(&cp.env_array);
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
> + cp.dir = path;
> + if (run_command(&cp)) {
> + warning(_("reading the index in submodule '%s' failed"),
> path);
The error is not (usually) in "reading the index" -- it's "updating the index" (or the working tree)
> + child_process_clear(&cp);
> + return -1;
> + }
> +
> + /* write index to working dir */
> + child_process_clear(&cp);
> + child_process_init(&cp);
> + argv_array_pushl(&cp.args, "checkout-index", "-a", NULL);
I'm confused -- doesn't read-tree -u already do this? And if not, shouldn't we back out the result of the read-tree, to leave the submodule as it was?
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
> + cp.dir = path;
> + if (force)
> + argv_array_push(&cp.args, "-f");
> +
> + if (run_command(&cp)) {
> + warning(_("populating the working directory in submodule '%s'
> failed"), path);
> + child_process_clear(&cp);
> + return -1;
> + }
> +
> + /* get the HEAD right */
> + child_process_clear(&cp);
> + child_process_init(&cp);
> + argv_array_pushl(&cp.args, "checkout", "--recurse-submodules",
> NULL);
Why are we running checkout on the submodule when we've already done most of the checkout? The only thing left is to set HEAD and recurse, right? I must be missing something.
^ permalink raw reply
* Re: git describe number of commits different from git log count
From: Aaron Schrab @ 2016-12-05 23:27 UTC (permalink / raw)
To: Jan Hudec; +Cc: git
In-Reply-To: <1701823.75Syo8h0k0@box>
At 12:17 +0100 05 Feb 2016, Jan Hudec <bulb@ucw.cz> wrote:
>I have a repository with following situation:
>
> $ git describe next
> v4.1-2196-g5a414d7
> $ git describe next --match=v4.2
> v4.2-4757-g5a414d7
>
>Since the tag with fewest commits since is selected, it appears logical.
>However, v4.2 is descendant of v4.1, so it does not make sense for it to have
>more commits since. And rev-list or log confirm that:
>
> $ git rev-list v4.1..next | wc -l
> 2196
> $ git rev-list v4.2..next | wc -l
> 1152
>
>The number of commits since v4.1 matches what the describe counted, but the
>number of commits since v4.2 does not.
I'm encountering what seems to be a similar issue. I'm working with the
`build` branch of https://github.com/aschrab/mutt currently at 65b7094.
Most of the commits in that repo come from a mercurial repository, but I
don't think that is really effecting things (other than being related to
the need to use the --tags option).
$ git describe --tags
mutt-1-7-1-rel-137-g65b7094
$ git describe --tags --match=mutt-1-7-2-rel
mutt-1-7-2-rel-6246-g65b7094
$ git rev-list --count mutt-1-7-2-rel..HEAD
126
$ git rev-list --count mutt-1-7-1-rel..HEAD
137
$ git --version
git version 2.10.2
The number of additional commits shown when I force the tag is
completely insane! That's nearly every commit that is part of that
branch:
1036$ git rev-list --count HEAD
6250
Both of the tags above should be reachable along the first-parent path.
If I add the `--first-parent` option to the describe command it picks
the expected tag and the number additional commits seems sane:
$ git describe --tags --first-parent
mutt-1-7-2-rel-14-g65b7094
^ permalink raw reply
* RE: [RFC PATCHv2 15/17] checkout: recurse into submodules if asked to
From: David Turner @ 2016-12-05 23:36 UTC (permalink / raw)
To: 'Stefan Beller', bmwill@google.com
Cc: git@vger.kernel.org, sandals@crustytoothpaste.net,
hvoigt@hvoigt.net, gitster@pobox.com
In-Reply-To: <20161203003022.29797-16-sbeller@google.com>
> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
> Sent: Friday, December 02, 2016 7:30 PM
> To: bmwill@google.com; David Turner
> Cc: git@vger.kernel.org; sandals@crustytoothpaste.net; hvoigt@hvoigt.net;
> gitster@pobox.com; Stefan Beller
> Subject: [RFC PATCHv2 15/17] checkout: recurse into submodules if asked to
>
> Allow checkout to recurse into submodules via the command line option --
> [no-]recurse-submodules.
This option probably needs to precede 9/17 "update submodules: add scheduling to update submodules", since that patch uses --recurse-submodules.
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Brandon Williams @ 2016-12-05 23:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, sbeller, bburky, jrnieder
In-Reply-To: <xmqqa8ca2cc7.fsf@gitster.mtv.corp.google.com>
On 12/05, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > I just took Jeff's series and applied it on top of mine (and fixed the
> > small problem causing t5812 to fail) and then rebased it to v2.9.0.
> > There were a few issues that needed to be resolved but nothing too
> > hairy. So it would definitely be doable to backport it to the
> > maintenance tracks.
>
> Thanks for trying. I'd definitely prefer a series that is based on
> an older codebase that is merged to pu->next->master->maint (in
> other words, avoid "backport" and rather have "forward merge").
Ah ok. Do you want me to send out the combined patch series based on
2.9.0 (or some other point in time) then?
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Junio C Hamano @ 2016-12-05 23:19 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jeff King, git, sbeller, bburky, jrnieder
In-Reply-To: <20161205222211.GF68588@google.com>
Brandon Williams <bmwill@google.com> writes:
> I just took Jeff's series and applied it on top of mine (and fixed the
> small problem causing t5812 to fail) and then rebased it to v2.9.0.
> There were a few issues that needed to be resolved but nothing too
> hairy. So it would definitely be doable to backport it to the
> maintenance tracks.
Thanks for trying. I'd definitely prefer a series that is based on
an older codebase that is merged to pu->next->master->maint (in
other words, avoid "backport" and rather have "forward merge").
^ permalink raw reply
* Re: [PATCHv1 1/2] git-p4: support git-workspaces
From: Junio C Hamano @ 2016-12-05 23:17 UTC (permalink / raw)
To: Luke Diamand; +Cc: Git Users, Vinicius Kursancew, Lars Schneider
In-Reply-To: <CAE5ih79efEu_2jgE9V-N7+UetyYu7RjH62whcfvMBtwM-Nb8Sg@mail.gmail.com>
Luke Diamand <luke@diamand.org> writes:
>> What I am trying to get at is if we want to use a single command
>> that can be given a path and answer "Yes, that is a repository"
>> here, and that single command should know how the repository should
>> look like. I offhand do not know we already have such a command we
>> can use, e.g. "git rev-parse --is-git-dir $path", but if there isn't
>> perhaps we would want one, so that not just "git p4" but other
>> scripted Porcelains can make use of it?
>
> That would be nicer than random ad-hoc rules, certainly. I couldn't
> find any obvious combination that would work.
I've already sent an update but my reading of this code is that
there is no need for the program to be able to, given a random
directory path, ask isValidGitDir() "is this a Git repository?" at
all.
What the program wanted to know, IIUC, is "Where is the 'Git
repository directory', if there is one?". This is needed primarily
because the program wants to error out before doing any operation
that requires a Git repository to work on, when the answer is "there
is none".
It also wants to know it because (for whatever reason) it wants to
export it in GIT_DIR [*1*].
And isValidGitDir() is a helper function used by the handcrafted
logic in main() to answer that question, but as far as I can tell,
you'd get a more correct answer by asking "rev-parse --git-dir"
unconditionally (even when the user of the program exported GIT_DIR
to you).
I just was reading Lars's LFS changes, but that one has hardcoded
"Is there a '.git/lfs/objects/' directory directly inside the
current working directory?" and similar, which I do not think would
work well in a secondary working tree where ".git" is merely a file
[*2*]. In a "secondary working tree"-aware world, I think you would
need to ask locations of --git-dir and --git-common-dir to rev-parse
upfront and choose which ones are ought to be shared entities across
the family of worktrees and which ones are to be private per
worktree. I suspect that LFS objects want to be shared across
working trees that share the object store, for example, which would
mean that "--git-dir" is not what Lars would want to use.
[Footnote]
*1* I do not think this is necessary. Git tools know how to find
the repository by first checking GIT_DIR environment and if it is
not set then by walking up the directory hierarchy just fine without
the program exporting GIT_DIR for them.
*2* The part that bases this path relative to getcwd() is OK, as the
start-up sequence in main() does a cdup to the top before everything
else.
^ permalink raw reply
* Re: Feature request: Set git svn options in .git/config file
From: Eric Wong @ 2016-12-05 22:54 UTC (permalink / raw)
To: Juergen Kosel; +Cc: git
In-Reply-To: <1936940c-c4c8-540c-eb99-b434e8d32d6c@gmx.de>
Juergen Kosel <juergen.kosel@gmx.de> wrote:
> Therefore I believe, that it would be the best solution to store the
> settings of --add-author-from, --use-log-author and maybe
> --authors-prog in the .git/config file.
Actually, "svn.authorsProg" is already documented as a config
option for --authors-prog.
It's been a while since I looked at this, but in git-svn,
all the "--xxx-yyy" command-line options should be available
under the appropriate "svn.xxxYyy" config key.
So, can you confirm that svn.addAuthorFrom and svn.useLogAuthor
config keys work and can be documented?
Even better would be for you to provide a patch to the
documentation :)
Otherwise, I can write one up sometime this week.
Thanks.
^ permalink raw reply
* Re: [PATCH] diff: fix up SHA-1 abbreviations outside of repository
From: Junio C Hamano @ 2016-12-05 22:43 UTC (permalink / raw)
To: Jeff King; +Cc: Jack Bates, git, Jack Bates
In-Reply-To: <20161205072614.zg6yglqnznna65vf@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I agree that it may be an accident waiting to happen, though, as soon as
> some buried sub-function needs to care about the distinction.
Yes to that.
>> I wonder if we're better off if we made sure that diff_no_index()
>> works the same way regardless of the value of "have_repository"
>> field?
>
> If you mean adding a diffopt flag like "just abbreviate everything to
> FALLBACK_DEFAULT_ABBREV even if we're in a repository", and then setting
> that in diff_no_index(), I agree that is a lot cleaner.
I am not sure if that is what I meant (I no longer sure what exactly
I meant to say there TBH), but this is probably not limited to the
default abbrev length aka core.abbrev configuration. Don't we have
other configuration settings we may read from $HOME/.gitconfig (and
possibly per-repository .git/config, if we did discovery but were
explicitly given "--no-index") that want to affect the behaviour of
the command?
I guess what I wanted, with "the same way", to see happen was that
"have_repository" should be only controling how and from what files
the configuration is read, and the behaviour of the command should
be controlled by the values read from the configuration after that.
Specifically, even if we were running with "--no-index", if we know
we have access to the current repository discovered by setting it up
gently, I do not think it is bad to ask find_unique_abbrev() to come
up with an appropriate abbreviation. So the fact that patch in
question has to flip the have_repository bit off, if it is done in
order to affect what diff_abbrev_oid() does, smells quite fishy from
that point of view.
^ permalink raw reply
* Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling
From: Ramsay Jones @ 2016-12-05 22:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, GIT Mailing-list
In-Reply-To: <xmqqr35m2eva.fsf@gitster.mtv.corp.google.com>
On 05/12/16 22:24, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>
>> As I said, the original version of the patch just removed the
>> --abbrev=7, but then I started to think about why you might have
>> used --abbrev in the first place (first in commit 9b88fcef7 and
>> again in commit bf505158d). Making sure to override the configuration
>> was the only thing I could come up with. So, I was hoping you could
>> remember why! :-P
>
> Nope. As a maintainer support script, the only thing I cared about
> it is that there is no -gXXXX at the end for anything I release ;-)
>
>> (I assumed it was to force a measure of uniformity/reproducibility).
>
> You cannot force uniformity/reproducibility with fixed abbrev,
> unless you set abbreviation length to 40, so you are correct to add
> "a measure of" there ;-)
Indeed. ;-)
> The first choice (i.e. 4) may have had a
> justification to force absolute minimum, and the second one (i.e. 7)
> may have had a justifiation to make it clear that we are using the
> same setting as the default, so in post-1.7.10 era, I think it is
> fine for us to just say "we have been using the same as default, so
> let's not specify anything explicitly".
So, you would be happy with just removing the '--abbrev' argument?
(That's fine by me; I don't set core.abbrev!)
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [RFC PATCHv2 17/17] checkout: add config option to recurse into submodules by default
From: Brandon Williams @ 2016-12-05 22:26 UTC (permalink / raw)
To: Stefan Beller
Cc: David Turner, git@vger.kernel.org, brian m. carlson, Heiko Voigt,
Junio C Hamano
In-Reply-To: <CAGZ79kaMXmNUaUCZJaTrcodAoSBPSxXr5JPrZGQeg=m-HrN11w@mail.gmail.com>
On 12/05, Stefan Beller wrote:
> On Mon, Dec 5, 2016 at 11:29 AM, Brandon Williams <bmwill@google.com> wrote:
> > On 12/02, Stefan Beller wrote:
> >> +test_expect_success 'option checkout.recurseSubmodules updates submodule' '
> >> + test_config checkout.recurseSubmodules 1 &&
> >> + git checkout base &&
> >> + git checkout -b advanced-base &&
> >> + git -C submodule commit --allow-empty -m "empty commit" &&
> >> + git add submodule &&
> >> + git commit -m "advance submodule" &&
> >> + git checkout base &&
> >> + git diff-files --quiet &&
> >> + git diff-index --quiet --cached base &&
> >> + git checkout advanced-base &&
> >> + git diff-files --quiet &&
> >> + git diff-index --quiet --cached advanced-base &&
> >> + git checkout --recurse-submodules base
> >> +'
> >> +
> >
> > This test doesn't look like it looks into the submodule to see if the
> > submodule has indeed changed. Unless diff-index and diff-files recurse
> > into the submodules?
>
> I took the code from Jens once upon a time. Rereading the code, I agree it is
> not obvious how this checks the submodule state.
>
> However `git diff-files --quiet` is perfectly fine, as
> we have submodule support by default via:
>
> Omitting the --submodule option or specifying --submodule=short,
> uses the short format. This format just shows the names of the commits
> at the beginning and end of the range.
>
> and then we turn it into an exit code via
>
> --quiet
> Disable all output of the program. Implies --exit-code.
>
> --exit-code
> Make the program exit with codes similar to diff(1).
> That is, it exits with 1 if there were differences and 0 means no differences.
>
> Same for diff-index.
>
> The main purpose of this specific test is to have checkout.recurseSubmodules
> "to just make it work" without having to give --recurse-submodules manually.
> All the other tests with the manual --recurse-submodules should test for
> correctness of the behavior within the submodule.
>
> So maybe I'll need to rewrite submodule_creation_must_succeed() in the previous
> patch to be more obvious. (Well that already has some tests for
> files/directories
> in there, so it is a little more.)
>
> But to be sure we can also add tests here that look more into the submodule.
> I am thinking of "{new,old}_sub_sha1=$(git -C submodule rev-parse HEAD)" and
> comparing them?
Ah ok, that makes sense now. Its kind of like if I run git status it
would show if a submodule is at a different sha1 than the superproject
has recorded.
--
Brandon Williams
^ permalink raw reply
* [PATCH] difftool: fix dir-diff index creation when in a subdirectory
From: David Aguilar @ 2016-12-05 22:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git ML, Frank Becker, Johannes Schindelin, John Keeping
9ec26e797781239b36ebccb87c590e5778358007 corrected how path arguments
are handled in a subdirectory, but it introduced a regression in how
entries outside of the subdirectory are handled by dir-diff.
When preparing the right-side of the diff we only include the
changed paths in the temporary area.
The left side of the diff is constructed from a temporary
index that is built from the same set of changed files, but it
was being constructed from within the subdirectory. This is a
problem because the indexed paths are toplevel-relative, and
thus they were not getting added to the index.
Teach difftool to chdir to the toplevel of the repository before
preparing its temporary indexes. This ensures that all of the
toplevel-relative paths are valid.
Adjust the test cases to more thoroughly exercise this scenario.
Reported-by: Frank Becker <fb@mooflu.com>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
I figured I'd send this as a proper patch.
The patch contents have not changed since the original in-thread
patch, but the commit message was modified slightly.
git-difftool.perl | 4 ++++
t/t7800-difftool.sh | 7 +++++--
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index a5790d03a..959822d5f 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -182,6 +182,10 @@ EOF
}
}
+ # Go to the root of the worktree so that the left index files
+ # are properly setup -- the index is toplevel-relative.
+ chdir($workdir);
+
# Setup temp directories
my $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 0, TMPDIR => 1);
my $ldir = "$tmpdir/left";
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 70a2de461..caab4b5ca 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -413,8 +413,11 @@ run_dir_diff_test 'difftool --dir-diff from subdirectory' '
(
cd sub &&
git difftool --dir-diff $symlinks --extcmd ls branch >output &&
- grep sub output &&
- grep file output
+ # "sub" must only exist in "right"
+ # "file" and "file2" must be listed in both "left" and "right"
+ test "1" = $(grep sub output | wc -l) &&
+ test "2" = $(grep file"$" output | wc -l) &&
+ test "2" = $(grep file2 output | wc -l)
)
'
--
2.11.0
^ permalink raw reply related
* Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling
From: Junio C Hamano @ 2016-12-05 22:24 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Jeff King, GIT Mailing-list
In-Reply-To: <e74163ee-e7d4-bcbd-e65f-368bc2ee9a2d@ramsayjones.plus.com>
Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> As I said, the original version of the patch just removed the
> --abbrev=7, but then I started to think about why you might have
> used --abbrev in the first place (first in commit 9b88fcef7 and
> again in commit bf505158d). Making sure to override the configuration
> was the only thing I could come up with. So, I was hoping you could
> remember why! :-P
Nope. As a maintainer support script, the only thing I cared about
it is that there is no -gXXXX at the end for anything I release ;-)
> (I assumed it was to force a measure of uniformity/reproducibility).
You cannot force uniformity/reproducibility with fixed abbrev,
unless you set abbreviation length to 40, so you are correct to add
"a measure of" there ;-) The first choice (i.e. 4) may have had a
justification to force absolute minimum, and the second one (i.e. 7)
may have had a justifiation to make it clear that we are using the
same setting as the default, so in post-1.7.10 era, I think it is
fine for us to just say "we have been using the same as default, so
let's not specify anything explicitly".
^ permalink raw reply
* Re: [RFC PATCHv2 17/17] checkout: add config option to recurse into submodules by default
From: Stefan Beller @ 2016-12-05 22:23 UTC (permalink / raw)
To: Brandon Williams
Cc: David Turner, git@vger.kernel.org, brian m. carlson, Heiko Voigt,
Junio C Hamano
In-Reply-To: <20161205192918.GB68588@google.com>
On Mon, Dec 5, 2016 at 11:29 AM, Brandon Williams <bmwill@google.com> wrote:
> On 12/02, Stefan Beller wrote:
>> +test_expect_success 'option checkout.recurseSubmodules updates submodule' '
>> + test_config checkout.recurseSubmodules 1 &&
>> + git checkout base &&
>> + git checkout -b advanced-base &&
>> + git -C submodule commit --allow-empty -m "empty commit" &&
>> + git add submodule &&
>> + git commit -m "advance submodule" &&
>> + git checkout base &&
>> + git diff-files --quiet &&
>> + git diff-index --quiet --cached base &&
>> + git checkout advanced-base &&
>> + git diff-files --quiet &&
>> + git diff-index --quiet --cached advanced-base &&
>> + git checkout --recurse-submodules base
>> +'
>> +
>
> This test doesn't look like it looks into the submodule to see if the
> submodule has indeed changed. Unless diff-index and diff-files recurse
> into the submodules?
I took the code from Jens once upon a time. Rereading the code, I agree it is
not obvious how this checks the submodule state.
However `git diff-files --quiet` is perfectly fine, as
we have submodule support by default via:
Omitting the --submodule option or specifying --submodule=short,
uses the short format. This format just shows the names of the commits
at the beginning and end of the range.
and then we turn it into an exit code via
--quiet
Disable all output of the program. Implies --exit-code.
--exit-code
Make the program exit with codes similar to diff(1).
That is, it exits with 1 if there were differences and 0 means no differences.
Same for diff-index.
The main purpose of this specific test is to have checkout.recurseSubmodules
"to just make it work" without having to give --recurse-submodules manually.
All the other tests with the manual --recurse-submodules should test for
correctness of the behavior within the submodule.
So maybe I'll need to rewrite submodule_creation_must_succeed() in the previous
patch to be more obvious. (Well that already has some tests for
files/directories
in there, so it is a little more.)
But to be sure we can also add tests here that look more into the submodule.
I am thinking of "{new,old}_sub_sha1=$(git -C submodule rev-parse HEAD)" and
comparing them?
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Brandon Williams @ 2016-12-05 22:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, sbeller, bburky, jrnieder
In-Reply-To: <xmqqr35m3zx7.fsf@gitster.mtv.corp.google.com>
On 12/05, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > On 12/01, Junio C Hamano wrote:
> >> Brandon Williams <bmwill@google.com> writes:
> >>
> >> > I started taking a look at your http redirect series (I really should
> >> > have taking a look at it sooner) and I see exactly what you're talking
> >> > about. We can easily move this logic into a function to make it easier
> >> > to generate the two whitelists.
> >>
> >> OK. With both of them queued, t5812 seems to barf, just FYI; I'll
> >> expect that a future reroll would make things better.
> >
> > Yeah I expected we would see an issue since both these series collide in
> > http.c
> >
> > I'm sending out another reroll of this series so that in Jeff's he can
> > just call 'get_curl_allowed_protocols(-1)' for the non-redirection curl
> > option, which should make this test stop barfing.
>
> I was hoping to eventually merge Peff's series to older maintenance
> tracks. How bad would it be if we rebased the v8 of this series
> together with Peff's series to say v2.9 (or even older if it does
> not look too bad)?
I just took Jeff's series and applied it on top of mine (and fixed the
small problem causing t5812 to fail) and then rebased it to v2.9.0.
There were a few issues that needed to be resolved but nothing too
hairy. So it would definitely be doable to backport it to the
maintenance tracks.
--
Brandon Williams
^ permalink raw reply
* Re: git repo vs project level authorization
From: Junio C Hamano @ 2016-12-05 21:59 UTC (permalink / raw)
To: ken edward; +Cc: git
In-Reply-To: <CAAqgmoO+7cLZHpX61=Mh7PjqrCUc0qyFD=C+sjVat_+KPhisbw@mail.gmail.com>
ken edward <kedward777@gmail.com> writes:
> I am currently using svn with apache+mod_dav_svn to have a single
> repository with multiple projects. Each of the projects is controlled
> by an access control file that lists the project path and the allowed
> usernames.
>
> Does git have this also? where is the doc?
It is customary not to mix unrelated projects into a single
repository in the Git land. Some hosting solutions give access
control per branches, and some others give access control per
repositories.
^ permalink raw reply
* Re: git repo vs project level authorization
From: Fredrik Gustafsson @ 2016-12-05 22:04 UTC (permalink / raw)
To: ken edward; +Cc: git
In-Reply-To: <CAAqgmoO+7cLZHpX61=Mh7PjqrCUc0qyFD=C+sjVat_+KPhisbw@mail.gmail.com>
On Mon, Dec 05, 2016 at 03:33:51PM -0500, ken edward wrote:
> I am currently using svn with apache+mod_dav_svn to have a single
> repository with multiple projects. Each of the projects is controlled
> by an access control file that lists the project path and the allowed
> usernames.
>
> Does git have this also? where is the doc?
>
> Ken
Git does not do hosting or access control. For this you need to use a
third party program. There are plenty of options for you and each has
different features and limitations. For example you should take a look
at gitolite, gitlab, bitbucket, github, gogs. Just to mention a few.
It's also possible to setup git with ssh or http/https with your own
access control methods. See the progit book for details here.
--
Fredrik Gustafsson
phone: +46 733-608274
e-mail: iveqy@iveqy.com
website: http://www.iveqy.com
^ permalink raw reply
* Re: [PATCHv1 1/2] git-p4: support git-workspaces
From: Luke Diamand @ 2016-12-05 21:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Users, Vinicius Kursancew, Lars Schneider
In-Reply-To: <xmqq4m2i3w8b.fsf@gitster.mtv.corp.google.com>
On 5 December 2016 at 21:24, Junio C Hamano <gitster@pobox.com> wrote:
> Luke Diamand <luke@diamand.org> writes:
>
>> On 5 December 2016 at 20:53, Junio C Hamano <gitster@pobox.com> wrote:
>>> Luke Diamand <luke@diamand.org> writes:
>>>
>>>> Teach git-p4 about git-workspaces.
>>>
>>> Is this what we call "git worktree", or something else?
>>
>> Ah, I think you're right!
>
> Then I'll queue it like the attached.
>
> HOWEVER.
>
> How fast does isValidGitDir() function need to be?
It doesn't need to be fast.
> The primary one
> seems to check HEAD (but it does not notice a non-repository that
> has a directory with that name), refs and objects (but it does not
> notice a non-repository that has a non-directory with these names),
> and this new one uses a test that is even more sloppy.
>
> What I am trying to get at is if we want to use a single command
> that can be given a path and answer "Yes, that is a repository"
> here, and that single command should know how the repository should
> look like. I offhand do not know we already have such a command we
> can use, e.g. "git rev-parse --is-git-dir $path", but if there isn't
> perhaps we would want one, so that not just "git p4" but other
> scripted Porcelains can make use of it?
That would be nicer than random ad-hoc rules, certainly. I couldn't
find any obvious combination that would work.
Luke
^ permalink raw reply
* Re: [PATCHv1 1/2] git-p4: support git-workspaces
From: Junio C Hamano @ 2016-12-05 21:54 UTC (permalink / raw)
To: Luke Diamand; +Cc: Git Users, Vinicius Kursancew, Lars Schneider
In-Reply-To: <xmqq4m2i3w8b.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Luke Diamand <luke@diamand.org> writes:
> ...
> if (os.path.exists(path + "/HEAD")
> and os.path.exists(path + "/refs") and os.path.exists(path + "/objects")):
> return True;
> +
> + # secondary working tree managed by "git worktree"?
> + if (os.path.exists(path + "/HEAD")
> + and os.path.exists(path + "/gitdir")):
> + return True
> +
> return False
Wait a bit. How is this expected to work?
I assume "path" is something like "$somewhere/.git" (either
absolute or relative), and by concatenating HEAD, refs and objects
under it the original checks form paths to expected directories,
because "$somewhere/.git" is a directory.
A worktree created with "git worktree add" gets ".git" that is a
file, and the contents of the file records the path to a
subdirectory "worktrees/$name" of the primary repository the
worktree is borrowing from, i.e. "$somewhere/.git/worktrees/$name".
So for this patch to work correctly, the caller, when in a "git
worktree" managed secondary working tree, MUST have found the ".git"
file at the root level of the working tree, MUST have read its
contents to learn that "$somewhere/.git/worktrees/$name" path, and
then feeding it to this function. But doesn't such a caller already
know that it is a valid repository? After all, it trusted the
contents of that ".git" file at the root level.
... goes and looks at the callers ...
Between the two call sites of isValidGitDir() in main(), the first
one (i.e. if ".git" in the current directory, made into abspath,
does not look like a repository, ask "rev-parse --git-dir" for one)
looks correct and I think it would grab the correct $GIT_DIR just
fine [*1*]. Isn't the real problem the second one, i.e. the one
that feeds a correct cmd.gitdir that we just obtained by asking
"rev-parse --git-dir" to the sloppy isValidGitDir() again. The
latter second guesses "rev-parse --git-dir" and botches.
I have a feeling that adding more to isValidGitDir() like this patch
does is a step in the wrong direction. The first fix would be not
to do the "if isValidGitDir() says no, then do something else" step
if you already asked "rev-parse --git-dir" and got a valid $GIT_DIR,
perhaps?
Stepping back even more.
I wonder why the script needs to do os.environ.get("GIT_DIR") in the
first place to initialize cmd.gitdir field. If it instead asked
"rev-parse --gitdir", the script would get the right answer that
already takes GIT_DIR environment into account.
[Footnote]
*1* This ends up asking "rev-parse --git-dir" only because
isValidGitDir() is sloppy. If you are in a secondary working
tree whose ".git" is a file, the function would say "no", and
that is the only thing that allows you to make a call to
"rev-parse --git-dir" to obtain the correct result.
^ permalink raw reply
* Re: [PATCHv1 1/2] git-p4: support git-workspaces
From: Junio C Hamano @ 2016-12-05 21:24 UTC (permalink / raw)
To: Luke Diamand; +Cc: Git Users, Vinicius Kursancew, Lars Schneider
In-Reply-To: <CAE5ih78Y_AbfgtW_6zMKLC8NzBxCKSagrgrjtfWZVOEwaAg6ZA@mail.gmail.com>
Luke Diamand <luke@diamand.org> writes:
> On 5 December 2016 at 20:53, Junio C Hamano <gitster@pobox.com> wrote:
>> Luke Diamand <luke@diamand.org> writes:
>>
>>> Teach git-p4 about git-workspaces.
>>
>> Is this what we call "git worktree", or something else?
>
> Ah, I think you're right!
Then I'll queue it like the attached.
HOWEVER.
How fast does isValidGitDir() function need to be? The primary one
seems to check HEAD (but it does not notice a non-repository that
has a directory with that name), refs and objects (but it does not
notice a non-repository that has a non-directory with these names),
and this new one uses a test that is even more sloppy.
What I am trying to get at is if we want to use a single command
that can be given a path and answer "Yes, that is a repository"
here, and that single command should know how the repository should
look like. I offhand do not know we already have such a command we
can use, e.g. "git rev-parse --is-git-dir $path", but if there isn't
perhaps we would want one, so that not just "git p4" but other
scripted Porcelains can make use of it?
-- >8 --
From: Luke Diamand <luke@diamand.org>
Date: Fri, 2 Dec 2016 22:43:18 +0000
Subject: [PATCH] git-p4: support secondary working trees managed by "git worktree"
Teach git-p4 about them.
Signed-off-by: Luke Diamand <luke@diamand.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
git-p4.py | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/git-p4.py b/git-p4.py
index fd5ca52462..b3c50ae7e5 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -566,6 +566,12 @@ def isValidGitDir(path):
if (os.path.exists(path + "/HEAD")
and os.path.exists(path + "/refs") and os.path.exists(path + "/objects")):
return True;
+
+ # secondary working tree managed by "git worktree"?
+ if (os.path.exists(path + "/HEAD")
+ and os.path.exists(path + "/gitdir")):
+ return True
+
return False
def parseRevision(ref):
--
2.11.0-222-g22b1346184
^ 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