* Re: git add -p with unmerged files
From: Junio C Hamano @ 2016-12-13 19:59 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Ariel, git, Duy Nguyen, Jeff King
In-Reply-To: <98817141-fa57-7687-09c4-dc96419d8a35@gmx.net>
Stephan Beyer <s-beyer@gmx.net> writes:
> While we're on the topic that "git add -p" should behave like the
> "normal" "git add" (not "git add -u"): what about unmerged changes?
>
>
> When I have merge conflicts, I almost always use my aliases
> "edit-unmerged" and "add-unmerged":
>
> $ git config --global --list | grep unmerged
> alias.list-unmerged=diff --name-only --diff-filter=U
> alias.edit-unmerged=!vim `git list-unmerged`
> alias.add-unmerged=!git add `git list-unmerged`
> alias.reset-unmerged=!uf=`git list-unmerged`; git reset HEAD $uf; git
> checkout -- $uf
>
> The "add-unmerged" alias is always a little scary because I'd rather
> like to check the changes with the "git add -p" workflow I am used to.
>
> Opinions?
For this, you would NEVER want to use "add -p" to pick and choose.
By definition, while you are in conflicted merge, the path that had
conflicts before you started the merge-y operation (be it "pull",
"am -3", or "cherry-pick") did not have any change since HEAD, and
"pick this hunk, drop that hunk" cannot be correct for the conflict
resolution.
"git diff" while conflicted will highlight what conflicted by
showing the three-way diff (similar to "diff --cc" on a merge
result) and after conflict is resolved you can view "diff HEAD"
on the path to see what the merge brought in.
^ permalink raw reply
* Re: [RFC/PATCH v3 00/16] Add initial experimental external ODB support
From: Junio C Hamano @ 2016-12-13 20:05 UTC (permalink / raw)
To: Christian Couder
Cc: git, Jeff King, Nguyen Thai Ngoc Duy, Mike Hommey, Lars Schneider,
Eric Wong, Christian Couder
In-Reply-To: <CAP8UFD1=-xKWaDnGKrtm2mzVxpH7N-Q3iqnOJeOM5QrtNpitrA@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> In general I think that having a lot of refs is really a big problem
> right now in Git as many big organizations using Git are facing this
> problem in one form or another.
> So I think that support for a big number of refs is a separate and
> important problem that should and hopefully will be solved.
But you do not have to make it worse.
Is "refs" a good match for the problem you are solving? Or is it
merely an expedient thing to use? I think it is the latter, judging
by your mentioning RefTree. Whatever mechanism we choose, that will
be carved into stone in users' repositories and you'd end up having
to support it, and devise the migration path out of it if the initial
selection is too problematic.
That is why people (not just me) pointed out upfront that using refs
for this purose would not scale.
^ permalink raw reply
* Re: [PATCH 2/2] tmp-objdir: quote paths we add to alternates
From: Junio C Hamano @ 2016-12-13 20:10 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, Klaus Ethgen, git
In-Reply-To: <20161213181538.6gv4it4b33uhbuud@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Dec 13, 2016 at 10:10:04AM -0800, Junio C Hamano wrote:
>
>> > - git clone --bare . xxx:yyy.git &&
>> > + git clone --bare . xxx${path_sep}yyy.git &&
>>
>> Don't you want to dq the whole thing to prevent the shell from
>> splitting this into two commands at ';'? The other one below is OK.
>
> After expansion, I don't think the shell will do any further processing
> except for whitespace splitting. E.g.:
Ah, my mistake. Staring at too many `eval`s does it to me.
Thanks.
^ permalink raw reply
* Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
From: Stefan Beller @ 2016-12-13 20:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, David Turner, Brandon Williams
In-Reply-To: <xmqq60mn3937.fsf@gitster.mtv.corp.google.com>
On Tue, Dec 13, 2016 at 11:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> The desired standard for submodules is to have the git dir inside the
>> superprojects git dir (since 501770e, Aug 2011, Move git-dir for
>> submodules), which is why I think an "embedded submodule git dir"
>> is inside the superproject already.
>
> Think how you start a new submodule. What are the steps you take
> before you say "git submodule add"? And where does .git for the
> submodule live at that point?
Well there is no way to directly start a submodule
(e.g. "git submodule create"), such that there has to be one repo
that actually has the git dir inside the working tree.
If the submodule exists already somewhere, there are 2 ways to do it
("git submodule add <URL>" or "git clone && git submodule add")
which lead to different outcomes, where the .git resides.
> With the current system, you as the submodule originator need to do
> something different to make your working tree of the superproject
> match what the others who clone from your public repository.
>
> And comparing the two layout, the one originally held by the
> submodule originator has .git embedded in the working tree, no?
When starting a new submodule repo, yes, the git dir is inside
the working tree.
> All of the above is coming from "submodule" centric mindset. It
> just is not centric to those who follow what others originated.
ok.
> Another reason why I personally see a .git in each submodule working
> tree is "embedded" has nothing to do with Git. It is an analogy I
> feel (perhaps it is just me) with the word "embedded reporters in
> warzone". These people are spread around, assigned to units to
> report from places closer to the front line and being closer to the
> leaf of the hierarchy, as opposed to be assigned to a more central
> place like HQ to do their reporting.
I talked to people in the office and got a heavy rejection on the
the work "embedded" here for another reason:
"Does it put the submodule on an embedded device?
What does embedded even mean?
The end user is super confused"
So I think we should not use embed or un-embed one way or the other.
Instead we need to have another name.
I think absorb is ok-ish, as "git submodule absorb" hints that the
superproject absorbs something from the submodule.
So I will reroll it with "absorb" fixing some nits pointed out by David?
^ permalink raw reply
* Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
From: Stefan Beller @ 2016-12-13 20:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, David Turner, Brandon Williams
In-Reply-To: <CAGZ79kYM_3NWyRfk42=EshMYVZ=DSWRtn4RU4jkUE7v1EN6ngg@mail.gmail.com>
On Tue, Dec 13, 2016 at 12:09 PM, Stefan Beller <sbeller@google.com> wrote:
>
> So I will reroll it with "absorb" fixing some nits pointed out by David?
I got confused there, Davids nits are for this series, the absorb series itself
doesn't seem to have nits.
So I'll just reroll this series on top of the currently
sb/submodule-embed-gitdir (which you originally noted to be better renamed to
submodule-absorb-gitdir) merged with t3600-cleanup.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v2 02/34] sequencer (rebase -i): implement the 'noop' command
From: Junio C Hamano @ 2016-12-13 20:38 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <934c8e420cc4a75b1f3e4489fa4a4135c48f78ae.1481642927.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> +/*
> + * Note that ordering matters in this enum. Not only must it match the mapping
> + * below, it is also divided into several sections that matter. When adding
> + * new commands, make sure you add it in the right section.
> + */
Good thinking. Makes me wish C were a better language, though ;-)
> enum todo_command {
> + /* commands that handle commits */
> TODO_PICK = 0,
> - TODO_REVERT
> + TODO_REVERT,
> + /* commands that do nothing but are counted for reporting progress */
> + TODO_NOOP
> };
>
> static const char *todo_command_strings[] = {
> "pick",
> - "revert"
> + "revert",
> + "noop"
> };
> @@ -1292,7 +1316,12 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
> struct todo_item *item = todo_list->items + todo_list->current;
> if (save_todo(todo_list, opts))
> return -1;
> - res = do_pick_commit(item->command, item->commit, opts);
> + if (item->command <= TODO_REVERT)
> + res = do_pick_commit(item->command, item->commit,
> + opts);
> + else if (item->command != TODO_NOOP)
> + return error(_("unknown command %d"), item->command);
I wonder if making this a switch() statement is easier to read in
the longer run. The only thing at this point we are gaining by "not
only mapping and enum must match, the orders matter" is so that this
codepath can do the same thing for PICK and REVERT, but these two
would become more and more minority as we learn more words.
> todo_list->current++;
> if (res)
> return res;
^ permalink raw reply
* Re: [PATCH v2 01/34] sequencer: support a new action: 'interactive rebase'
From: Junio C Hamano @ 2016-12-13 20:32 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <297140020a7312af03136848dcdd0353ee3abdfe.1481642927.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> static inline int is_rebase_i(const struct replay_opts *opts)
> {
> - return 0;
> + return opts->action == REPLAY_INTERACTIVE_REBASE;
> }
>
> static const char *get_dir(const struct replay_opts *opts)
> {
> + if (is_rebase_i(opts))
> + return rebase_path();
> return git_path_seq_dir();
> }
>
> static const char *get_todo_path(const struct replay_opts *opts)
> {
> + if (is_rebase_i(opts))
> + return rebase_path_todo();
> return git_path_todo_file();
> }
>
> @@ -168,7 +179,15 @@ int sequencer_remove_state(struct replay_opts *opts)
>
> static const char *action_name(const struct replay_opts *opts)
> {
> - return opts->action == REPLAY_REVERT ? N_("revert") : N_("cherry-pick");
> + switch (opts->action) {
> + case REPLAY_REVERT:
> + return N_("revert");
> + case REPLAY_PICK:
> + return N_("cherry-pick");
> + case REPLAY_INTERACTIVE_REBASE:
> + return N_("rebase -i");
> + }
> + die(_("Unknown action: %d"), opts->action);
> }
This case statement which looks perfectly sensible---it says that
there are three equal modes the subsystem operates in.
This is just a mental note and not a suggestion to change anything
immediately, but it makes me wonder if git_dir/get_todo_path would
also want to do so, moving towards retiring is_rebase_i() which is
"everything else vs one oddball which is rebase-i" mindset.
> @@ -395,7 +414,10 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>
> if (active_cache_changed &&
> write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
> - /* TRANSLATORS: %s will be "revert" or "cherry-pick" */
> + /*
> + * TRANSLATORS: %s will be "revert", "cherry-pick" or
> + * "rebase -i".
> + */
IIRC, the "TRANSLATORS:" comment has to deviate from our coding
style due to tool limitation and has to be done like this:
> + /* TRANSLATORS: %s will be "revert", "cherry-pick" or
> + * "rebase -i".
> + */
> @@ -1204,6 +1226,9 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
> const char *todo_path = get_todo_path(opts);
> int next = todo_list->current, offset, fd;
>
> + if (is_rebase_i(opts))
> + next++;
> +
This is because...? Everybody else counts 0-based while rebase-i
counts from 1 or something?
> fd = hold_lock_file_for_update(&todo_lock, todo_path, 0);
> if (fd < 0)
> return error_errno(_("could not lock '%s'"), todo_path);
Everything else in the patch is understandable. This bit isn't
without explanation, at least to me.
^ permalink raw reply
* [PATCHv2 1/5] submodule.h: add extern keyword to functions
From: Stefan Beller @ 2016-12-13 20:56 UTC (permalink / raw)
To: gitster; +Cc: git, David.Turner, bmwill, sandals, Stefan Beller
In-Reply-To: <20161213205622.841-1-sbeller@google.com>
As the upcoming series will add a lot of functions to the submodule
header, let's first make the header consistent to the rest of the project
by adding the extern keyword to functions.
As per the CodingGuidelines we try to stay below 80 characters per line,
so adapt all those functions to stay below 80 characters that are already
using more than one line. Those function using just one line are better
kept in one line than breaking them up into multiple lines just for the
goal of staying below the character limit as it makes grepping
for functions easier if they are one liners.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
submodule.h | 55 ++++++++++++++++++++++++++++++-------------------------
1 file changed, 30 insertions(+), 25 deletions(-)
diff --git a/submodule.h b/submodule.h
index 6229054b99..61fb610749 100644
--- a/submodule.h
+++ b/submodule.h
@@ -29,50 +29,55 @@ struct submodule_update_strategy {
};
#define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}
-int is_staging_gitmodules_ok(void);
-int update_path_in_gitmodules(const char *oldpath, const char *newpath);
-int remove_path_from_gitmodules(const char *path);
-void stage_updated_gitmodules(void);
-void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
+extern int is_staging_gitmodules_ok(void);
+extern int update_path_in_gitmodules(const char *oldpath, const char *newpath);
+extern int remove_path_from_gitmodules(const char *path);
+extern void stage_updated_gitmodules(void);
+extern void set_diffopt_flags_from_submodule_config(struct diff_options *,
const char *path);
-int submodule_config(const char *var, const char *value, void *cb);
-void gitmodules_config(void);
-int parse_submodule_update_strategy(const char *value,
+extern int submodule_config(const char *var, const char *value, void *cb);
+extern void gitmodules_config(void);
+extern int parse_submodule_update_strategy(const char *value,
struct submodule_update_strategy *dst);
-const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
-void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
-void show_submodule_summary(FILE *f, const char *path,
+extern const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
+extern void handle_ignore_submodules_arg(struct diff_options *, const char *);
+extern void show_submodule_summary(FILE *f, const char *path,
const char *line_prefix,
struct object_id *one, struct object_id *two,
unsigned dirty_submodule, const char *meta,
const char *del, const char *add, const char *reset);
-void show_submodule_inline_diff(FILE *f, const char *path,
+extern void show_submodule_inline_diff(FILE *f, const char *path,
const char *line_prefix,
struct object_id *one, struct object_id *two,
unsigned dirty_submodule, const char *meta,
const char *del, const char *add, const char *reset,
const struct diff_options *opt);
-void set_config_fetch_recurse_submodules(int value);
-void check_for_new_submodule_commits(unsigned char new_sha1[20]);
-int fetch_populated_submodules(const struct argv_array *options,
+extern void set_config_fetch_recurse_submodules(int value);
+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,
int quiet, int max_parallel_jobs);
-unsigned is_submodule_modified(const char *path, int ignore_untracked);
-int submodule_uses_gitfile(const char *path);
-int ok_to_remove_submodule(const char *path);
-int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],
- const unsigned char a[20], const unsigned char b[20], int search);
-int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name,
- struct string_list *needs_pushing);
-int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);
-int parallel_submodules(void);
+extern unsigned is_submodule_modified(const char *path, int ignore_untracked);
+extern int submodule_uses_gitfile(const char *path);
+extern int ok_to_remove_submodule(const char *path);
+extern int merge_submodule(unsigned char result[20], const char *path,
+ const unsigned char base[20],
+ const unsigned char a[20],
+ const unsigned char b[20], int search);
+extern int find_unpushed_submodules(unsigned char new_sha1[20],
+ const char *remotes_name,
+ struct string_list *needs_pushing);
+extern int push_unpushed_submodules(unsigned char new_sha1[20],
+ const char *remotes_name);
+extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
+extern int parallel_submodules(void);
/*
* Prepare the "env_array" parameter of a "struct child_process" for executing
* a submodule by clearing any repo-specific envirionment variables, but
* retaining any config in the environment.
*/
-void prepare_submodule_repo_env(struct argv_array *out);
+extern void prepare_submodule_repo_env(struct argv_array *out);
#define ABSORB_GITDIR_RECURSE_SUBMODULES (1<<0)
extern void absorb_git_dir_into_superproject(const char *prefix,
--
2.11.0.rc2.35.g26e18c9
^ permalink raw reply related
* [PATCHv2 0/5] git-rm absorbs submodule git directory before deletion
From: Stefan Beller @ 2016-12-13 20:56 UTC (permalink / raw)
To: gitster; +Cc: git, David.Turner, bmwill, sandals, Stefan Beller
v2:
* new base where to apply the patch:
sb/submodule-embed-gitdir merged with sb/t3600-cleanup.
I got merge conflicts and resolved them this way:
#@@@ -709,9 -687,10 +687,9 @@@ test_expect_success 'checking out a com
# git commit -m "submodule removal" submod &&
# git checkout HEAD^ &&
# git submodule update &&
#- git checkout -q HEAD^ 2>actual &&
#+ git checkout -q HEAD^ &&
# git checkout -q master 2>actual &&
# - echo "warning: unable to rmdir submod: Directory not empty" >expected &&
# - test_i18ncmp expected actual &&
# + test_i18ngrep "^warning: unable to rmdir submod:" actual &&
# git status -s submod >actual &&
# echo "?? submod/" >expected &&
# test_cmp expected actual &&
#
* improved commit message in "ok_to_remove_submodule: absorb the submodule git dir"
(David Turner offered me some advice on how to write better English off list)
* simplified code in last patch:
-> dropped wrong comment for fallthrough
-> moved redundant code out of both bodies of an if-clause.
* Fixed last patchs commit message to have "or_die" instead of or_dir.
v1:
The "checkout --recurse-submodules" series got too large to comfortably send
it out for review, so I had to break it up into smaller series'; this is the
first subseries, but it makes sense on its own.
This series teaches git-rm to absorb the git directory of a submodule instead
of failing and complaining about the git directory preventing deletion.
It applies on origin/sb/submodule-embed-gitdir.
Any feedback welcome!
Thanks,
Stefan
Stefan Beller (5):
submodule.h: add extern keyword to functions
submodule: modernize ok_to_remove_submodule to use argv_array
submodule: add flags to ok_to_remove_submodule
ok_to_remove_submodule: absorb the submodule git dir
rm: add absorb a submodules git dir before deletion
builtin/rm.c | 33 ++++++++-----------------
cache.h | 2 ++
entry.c | 5 ++++
submodule.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++----------
submodule.h | 64 ++++++++++++++++++++++++++++++-------------------
t/t3600-rm.sh | 39 ++++++++++++------------------
6 files changed, 135 insertions(+), 85 deletions(-)
--
2.11.0.rc2.35.g26e18c9
^ permalink raw reply
* [PATCHv2 2/5] submodule: modernize ok_to_remove_submodule to use argv_array
From: Stefan Beller @ 2016-12-13 20:56 UTC (permalink / raw)
To: gitster; +Cc: git, David.Turner, bmwill, sandals, Stefan Beller
In-Reply-To: <20161213205622.841-1-sbeller@google.com>
Instead of constructing the NULL terminated array ourselves, we
should make use of the argv_array infrastructure.
While at it, adapt the error messages to reflect the actual invocation.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
submodule.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/submodule.c b/submodule.c
index 45ccfb7ab4..9f0b544ebe 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1023,13 +1023,6 @@ int ok_to_remove_submodule(const char *path)
{
ssize_t len;
struct child_process cp = CHILD_PROCESS_INIT;
- const char *argv[] = {
- "status",
- "--porcelain",
- "-u",
- "--ignore-submodules=none",
- NULL,
- };
struct strbuf buf = STRBUF_INIT;
int ok_to_remove = 1;
@@ -1039,14 +1032,15 @@ int ok_to_remove_submodule(const char *path)
if (!submodule_uses_gitfile(path))
return 0;
- cp.argv = argv;
+ argv_array_pushl(&cp.args, "status", "--porcelain", "-u",
+ "--ignore-submodules=none", NULL);
prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.out = -1;
cp.dir = path;
if (start_command(&cp))
- die("Could not run 'git status --porcelain -uall --ignore-submodules=none' in submodule %s", path);
+ die(_("could not run 'git status --porcelain -u --ignore-submodules=none' in submodule %s"), path);
len = strbuf_read(&buf, cp.out, 1024);
if (len > 2)
@@ -1054,7 +1048,7 @@ int ok_to_remove_submodule(const char *path)
close(cp.out);
if (finish_command(&cp))
- die("'git status --porcelain -uall --ignore-submodules=none' failed in submodule %s", path);
+ die(_("'git status --porcelain -u --ignore-submodules=none' failed in submodule %s"), path);
strbuf_release(&buf);
return ok_to_remove;
--
2.11.0.rc2.35.g26e18c9
^ permalink raw reply related
* [PATCHv2 4/5] ok_to_remove_submodule: absorb the submodule git dir
From: Stefan Beller @ 2016-12-13 20:56 UTC (permalink / raw)
To: gitster; +Cc: git, David.Turner, bmwill, sandals, Stefan Beller
In-Reply-To: <20161213205622.841-1-sbeller@google.com>
It is a major reason to say no, when deciding if a submodule can be
deleted, if the git directory of the submodule being contained in the
submodule's working directory.
Migrate the git directory into the superproject instead of failing,
and proceed with the other checks.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
submodule.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/submodule.c b/submodule.c
index 2d13744b06..e42efa2337 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1026,11 +1026,22 @@ int ok_to_remove_submodule(const char *path, unsigned flags)
struct strbuf buf = STRBUF_INIT;
int ok_to_remove = 1;
+ /* Is it there? */
if (!file_exists(path) || is_empty_dir(path))
return 1;
- if (!submodule_uses_gitfile(path))
- return 0;
+ /* Does it have a .git directory? */
+ if (!submodule_uses_gitfile(path)) {
+ absorb_git_dir_into_superproject("", path,
+ ABSORB_GITDIR_RECURSE_SUBMODULES);
+
+ /*
+ * We should be using a gitfile by now. Let's double
+ * check as losing the git dir would be fatal.
+ */
+ if (!submodule_uses_gitfile(path))
+ return 0;
+ }
argv_array_pushl(&cp.args, "status", "--porcelain",
"--ignore-submodules=none", NULL);
--
2.11.0.rc2.35.g26e18c9
^ permalink raw reply related
* [PATCHv2 5/5] rm: add absorb a submodules git dir before deletion
From: Stefan Beller @ 2016-12-13 20:56 UTC (permalink / raw)
To: gitster; +Cc: git, David.Turner, bmwill, sandals, Stefan Beller
In-Reply-To: <20161213205622.841-1-sbeller@google.com>
When deleting a submodule, we need to keep the actual git directory around,
such that we do not lose local changes in there and at a later checkout
of the submodule we don't need to clone it again.
Implement `depopulate_submodule`, that migrates the git directory before
deletion of a submodule and afterwards the equivalent of "rm -rf", which
is already found in entry.c, so expose that and for clarity add a suffix
"_or_die" to it.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
builtin/rm.c | 30 ++++++++----------------------
cache.h | 2 ++
entry.c | 5 +++++
submodule.c | 31 +++++++++++++++++++++++++++++++
submodule.h | 6 ++++++
t/t3600-rm.sh | 39 +++++++++++++++------------------------
6 files changed, 67 insertions(+), 46 deletions(-)
diff --git a/builtin/rm.c b/builtin/rm.c
index fdd7183f61..8c9c535a88 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -392,28 +392,14 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
for (i = 0; i < list.nr; i++) {
const char *path = list.entry[i].name;
if (list.entry[i].is_submodule) {
- if (is_empty_dir(path)) {
- if (!rmdir(path)) {
- removed = 1;
- if (!remove_path_from_gitmodules(path))
- gitmodules_modified = 1;
- continue;
- }
- } else {
- strbuf_reset(&buf);
- strbuf_addstr(&buf, path);
- if (!remove_dir_recursively(&buf, 0)) {
- removed = 1;
- if (!remove_path_from_gitmodules(path))
- gitmodules_modified = 1;
- strbuf_release(&buf);
- continue;
- } else if (!file_exists(path))
- /* Submodule was removed by user */
- if (!remove_path_from_gitmodules(path))
- gitmodules_modified = 1;
- /* Fallthrough and let remove_path() fail. */
- }
+ if (is_empty_dir(path) && rmdir(path))
+ die_errno("git rm: '%s'", path);
+ if (file_exists(path))
+ depopulate_submodule(path);
+ removed = 1;
+ if (!remove_path_from_gitmodules(path))
+ gitmodules_modified = 1;
+ continue;
}
if (!remove_path(path)) {
removed = 1;
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 e42efa2337..3770ecb7b9 100644
--- a/submodule.c
+++ b/submodule.c
@@ -308,6 +308,37 @@ static void print_submodule_summary(struct rev_info *rev, FILE *f,
strbuf_release(&sb);
}
+void depopulate_submodule(const char *path)
+{
+ 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)) {
+ absorb_git_dir_into_superproject("", path,
+ ABSORB_GITDIR_RECURSE_SUBMODULES);
+
+ if (!submodule_uses_gitfile(path)) {
+ /*
+ * We should be using a gitfile by now. Let's double
+ * check as losing the git dir would be fatal.
+ */
+ die("BUG: could not absorb git directory for '%s'", path);
+ }
+ }
+
+ strbuf_addstr(&pathbuf, path);
+ remove_directory_or_die(&pathbuf);
+
+out:
+ strbuf_release(&pathbuf);
+ free(dot_git);
+}
+
/* 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 3ed3aa479a..516e377a12 100644
--- a/submodule.h
+++ b/submodule.h
@@ -53,6 +53,12 @@ extern void show_submodule_inline_diff(FILE *f, const char *path,
const char *del, const char *add, const char *reset,
const struct diff_options *opt);
extern void set_config_fetch_recurse_submodules(int value);
+
+/*
+ * Removes a submodule from a given path. When the submodule contains its
+ * git directory instead of a gitlink, migrate that first into the superproject.
+ */
+extern void 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,
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index bcbb680651..5aa6db584c 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -569,26 +569,22 @@ test_expect_success 'rm of a conflicted unpopulated submodule succeeds' '
test_cmp expect actual
'
-test_expect_success 'rm of a populated submodule with a .git directory fails even when forced' '
+test_expect_success 'rm of a populated submodule with a .git directory migrates git dir' '
git checkout -f master &&
git reset --hard &&
git submodule update &&
(cd submod &&
rm .git &&
cp -R ../.git/modules/sub .git &&
- GIT_WORK_TREE=. git config --unset core.worktree
+ GIT_WORK_TREE=. git config --unset core.worktree &&
+ rm -r ../.git/modules/sub
) &&
- test_must_fail git rm submod &&
- test -d submod &&
- test -d submod/.git &&
- git status -s -uno --ignore-submodules=none >actual &&
- ! test -s actual &&
- test_must_fail git rm -f submod &&
- test -d submod &&
- test -d submod/.git &&
+ git rm submod 2>output.err &&
+ ! test -d submod &&
+ ! test -d submod/.git &&
git status -s -uno --ignore-submodules=none >actual &&
- ! test -s actual &&
- rm -rf submod
+ test -s actual &&
+ test_i18ngrep Migrating output.err
'
cat >expect.deepmodified <<EOF
@@ -667,24 +663,19 @@ test_expect_success 'rm of a populated nested submodule with a nested .git direc
git submodule update --recursive &&
(cd submod/subsubmod &&
rm .git &&
- cp -R ../../.git/modules/sub/modules/sub .git &&
+ mv ../../.git/modules/sub/modules/sub .git &&
GIT_WORK_TREE=. git config --unset core.worktree
) &&
- test_must_fail git rm submod &&
- test -d submod &&
- test -d submod/subsubmod/.git &&
- git status -s -uno --ignore-submodules=none >actual &&
- ! test -s actual &&
- test_must_fail git rm -f submod &&
- test -d submod &&
- test -d submod/subsubmod/.git &&
+ git rm submod 2>output.err &&
+ ! test -d submod &&
+ ! test -d submod/subsubmod/.git &&
git status -s -uno --ignore-submodules=none >actual &&
- ! test -s actual &&
- rm -rf submod
+ test -s actual &&
+ test_i18ngrep Migrating output.err
'
test_expect_success 'checking out a commit after submodule removal needs manual updates' '
- git commit -m "submodule removal" submod &&
+ git commit -m "submodule removal" submod .gitmodules &&
git checkout HEAD^ &&
git submodule update &&
git checkout -q HEAD^ &&
--
2.11.0.rc2.35.g26e18c9
^ permalink raw reply related
* [PATCHv2 3/5] submodule: add flags to ok_to_remove_submodule
From: Stefan Beller @ 2016-12-13 20:56 UTC (permalink / raw)
To: gitster; +Cc: git, David.Turner, bmwill, sandals, Stefan Beller
In-Reply-To: <20161213205622.841-1-sbeller@google.com>
In different contexts the question whether deleting a submodule is ok
to remove may be answered differently.
In 293ab15eea (submodule: teach rm to remove submodules unless they
contain a git directory, 2012-09-26) a case was made that we can safely
ignore ignored untracked files for removal as we explicitely ask for the
removal of the submodule.
In a later patch we want to remove submodules even when the user doesn't
explicitly ask for it (e.g. checking out a tree-ish in which the submodule
doesn't exist). In that case we want to be more careful when it comes
to deletion of untracked files. As of this patch it is unclear how this
will be implemented exactly, so we'll offer flags in which the caller
can specify how the different untracked files ought to be handled.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
builtin/rm.c | 3 ++-
submodule.c | 23 +++++++++++++++++++----
submodule.h | 5 ++++-
3 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/builtin/rm.c b/builtin/rm.c
index 3f3e24eb36..fdd7183f61 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -187,7 +187,8 @@ static int check_local_mod(struct object_id *head, int index_only)
*/
if (ce_match_stat(ce, &st, 0) ||
(S_ISGITLINK(ce->ce_mode) &&
- !ok_to_remove_submodule(ce->name)))
+ !ok_to_remove_submodule(ce->name,
+ SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED)))
local_changes = 1;
/*
diff --git a/submodule.c b/submodule.c
index 9f0b544ebe..2d13744b06 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1019,7 +1019,7 @@ int submodule_uses_gitfile(const char *path)
return 1;
}
-int ok_to_remove_submodule(const char *path)
+int ok_to_remove_submodule(const char *path, unsigned flags)
{
ssize_t len;
struct child_process cp = CHILD_PROCESS_INIT;
@@ -1032,15 +1032,27 @@ int ok_to_remove_submodule(const char *path)
if (!submodule_uses_gitfile(path))
return 0;
- argv_array_pushl(&cp.args, "status", "--porcelain", "-u",
+ argv_array_pushl(&cp.args, "status", "--porcelain",
"--ignore-submodules=none", NULL);
+
+ if (flags & SUBMODULE_REMOVAL_IGNORE_UNTRACKED)
+ argv_array_push(&cp.args, "-uno");
+ else
+ argv_array_push(&cp.args, "-uall");
+
+ if (!(flags & SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED))
+ argv_array_push(&cp.args, "--ignored");
+
prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.out = -1;
cp.dir = path;
if (start_command(&cp))
- die(_("could not run 'git status --porcelain -u --ignore-submodules=none' in submodule %s"), path);
+ die(_("could not run 'git status --porcelain --ignore-submodules=none %s %s' in submodule '%s'"),
+ (flags & SUBMODULE_REMOVAL_IGNORE_UNTRACKED) ? "-uno" : "-uall",
+ (!(flags & SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED)) ? "--ignored" : "",
+ path);
len = strbuf_read(&buf, cp.out, 1024);
if (len > 2)
@@ -1048,7 +1060,10 @@ int ok_to_remove_submodule(const char *path)
close(cp.out);
if (finish_command(&cp))
- die(_("'git status --porcelain -u --ignore-submodules=none' failed in submodule %s"), path);
+ die(_("'git status --porcelain --ignore-submodules=none %s %s' failed in submodule '%s'"),
+ (flags & SUBMODULE_REMOVAL_IGNORE_UNTRACKED) ? "-uno" : "-uall",
+ (!(flags & SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED)) ? "--ignored" : "",
+ path);
strbuf_release(&buf);
return ok_to_remove;
diff --git a/submodule.h b/submodule.h
index 61fb610749..3ed3aa479a 100644
--- a/submodule.h
+++ b/submodule.h
@@ -59,7 +59,10 @@ extern int fetch_populated_submodules(const struct argv_array *options,
int quiet, int max_parallel_jobs);
extern unsigned is_submodule_modified(const char *path, int ignore_untracked);
extern int submodule_uses_gitfile(const char *path);
-extern int ok_to_remove_submodule(const char *path);
+
+#define SUBMODULE_REMOVAL_IGNORE_UNTRACKED (1<<0)
+#define SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED (1<<1)
+extern int ok_to_remove_submodule(const char *path, unsigned flags);
extern int merge_submodule(unsigned char result[20], const char *path,
const unsigned char base[20],
const unsigned char a[20],
--
2.11.0.rc2.35.g26e18c9
^ permalink raw reply related
* Re: [PATCH v2 02/34] sequencer (rebase -i): implement the 'noop' command
From: Linus Torvalds @ 2016-12-13 20:48 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Git Mailing List, Kevin Daudt,
Dennis Kaarsemaker
In-Reply-To: <xmqqfulr1s5z.fsf@gitster.mtv.corp.google.com>
On Tue, Dec 13, 2016 at 12:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> +/*
>> + * Note that ordering matters in this enum. Not only must it match the mapping
>> + * below, it is also divided into several sections that matter. When adding
>> + * new commands, make sure you add it in the right section.
>> + */
>
> Good thinking. Makes me wish C were a better language, though ;-)
Do this:
static const char *todo_command_strings[] = {
[TODO_PICK] = "pick",
[TODO_REVERT] = "revert",
[TODO_NOOP] = "noop:,
};
which makes the array be order-independent. You still need to make
sure you fill in all the entries, of course, but it tends to avoid at
least one gotcha, and it makes it more obvious how the two are tied
together.
Linus
^ permalink raw reply
* Re: [PATCH v2 03/34] sequencer (rebase -i): implement the 'edit' command
From: Junio C Hamano @ 2016-12-13 21:30 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <a1361151ad1dad8f4dc3c412c7ed30f625d67ba0.1481642927.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> @@ -43,6 +44,20 @@ static GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
> */
> static GIT_PATH_FUNC(rebase_path_author_script, "rebase-merge/author-script")
> /*
It is minor, but please have a blank line to separate the logical
blocks. If you have "comment for thing A" before "thing A", then
having a blank after that before "comment for thing B" and "thing B"
that follow would help. Otherwise "thing A" immediately followed by
"comment for thing B" are (mis)read together, leading to nonsense.
> + * When an "edit" rebase command is being processed, the SHA1 of the
> + * commit to be edited is recorded in this file. When "git rebase
> + * --continue" is executed, if there are any staged changes then they
> + * will be amended to the HEAD commit, but only provided the HEAD
> + * commit is still the commit to be edited. When any other rebase
> + * command is processed, this file is deleted.
> + */
> +static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend")
> +/*
> + * When we stop at a given patch via the "edit" command, this file contains
> + * the long commit name of the corresponding patch.
If you abbreviate an object name to 38-hex that is still long but
that is not full; if you meant full 40-hex, better spell it out as
"full"---that conveys useful information to programmers (e.g. they
can just use get_sha1_hex()).
But I think you are writing short_commit_name() to it? So perhaps
"an abbreviated commit object name"?
> @@ -1301,9 +1318,87 @@ static int save_opts(struct replay_opts *opts)
> return res;
> }
>
> +static int make_patch(struct commit *commit, struct replay_opts *opts)
> +{
> + struct strbuf buf = STRBUF_INIT;
> + struct rev_info log_tree_opt;
> + const char *commit_buffer = get_commit_buffer(commit, NULL), *subject, *p;
> + int res = 0;
> +
> + p = short_commit_name(commit);
> + if (write_message(p, strlen(p), rebase_path_stopped_sha(), 1) < 0)
> + return -1;
> +
> + strbuf_addf(&buf, "%s/patch", get_dir(opts));
> + memset(&log_tree_opt, 0, sizeof(log_tree_opt));
> + init_revisions(&log_tree_opt, NULL);
> + log_tree_opt.abbrev = 0;
> + log_tree_opt.diff = 1;
> + log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
> + log_tree_opt.disable_stdin = 1;
> + log_tree_opt.no_commit_id = 1;
> + log_tree_opt.diffopt.file = fopen(buf.buf, "w");
> + log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
> + if (!log_tree_opt.diffopt.file)
> + res |= error_errno(_("could not open '%s'"), buf.buf);
> + else {
> + res |= log_tree_commit(&log_tree_opt, commit);
> + fclose(log_tree_opt.diffopt.file);
> + }
> + strbuf_reset(&buf);
> + strbuf_addf(&buf, "%s/message", get_dir(opts));
> + if (!file_exists(buf.buf)) {
> + find_commit_subject(commit_buffer, &subject);
> + res |= write_message(subject, strlen(subject), buf.buf, 1);
> + unuse_commit_buffer(commit, commit_buffer);
> + }
> + strbuf_release(&buf);
> +
> + return res;
> +}
OK. This seems to match what scripted make_patch does in a handful
of lines. We probably should have given you a helper to reduce
boilerplate that sets up log_tree_opt so that this function does not
have to be this long, but that is a separate topic.
Does it matter output_format is set to FORMAT_PATCH here, though?
With --no-commit-id set, I suspect there is no log message or
authorship information given to the output.
As you are only interested in seeing the patch/diff in this file and
the log is stored in a separate "message" file, as long as "patch"
file gets the patch correctly, it is not a problem, but it just
looked strange to see FORMAT_PATCH there.
^ permalink raw reply
* [PATCH] fix pushing to //server/share/dir paths on Windows
From: Johannes Sixt @ 2016-12-13 21:32 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, Git Mailing List
normalize_path_copy() is not prepared to keep the double-slash of a
//server/share/dir kind of path, but treats it like a regular POSIX
style path and transforms it to /server/share/dir.
The bug manifests when 'git push //server/share/dir master' is run,
because tmp_objdir_add_as_alternate() uses the path in normalized
form when it registers the quarantine object database via
link_alt_odb_entries(). Needless to say that the directory cannot be
accessed using the wrongly normalized path.
Fix it by skipping all of the root part, not just a potential drive
prefix. offset_1st_component takes care of this, see the
implementation in compat/mingw.c::mingw_offset_1st_component().
There is a change in behavior: \\server\share is not transformed
into //server/share anymore, but all subsequent directory separators
are rewritten to '/'. This should not make a difference; Windows can
handle the mix. In the context of 'git push' this cannot be verified,
though, as there seems to be an independent bug that transforms the
double '\\' to a single '\' on the way.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Another long-standing bug uncovered by the quarantine series.
Dscho, it looks like this could fix the original report at
https://github.com/git-for-windows/git/issues/979
This patch should cook well because of the change in behavior.
I would not be surprised if there is some fall-out.
The other bug I'm alluding to, I still have to investigate. I do
not think that it can be counted as fall-out.
path.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/path.c b/path.c
index 52d889c88e..02dc70fb92 100644
--- a/path.c
+++ b/path.c
@@ -991,7 +991,7 @@ const char *remove_leading_path(const char *in, const char *prefix)
*
* Performs the following normalizations on src, storing the result in dst:
* - Ensures that components are separated by '/' (Windows only)
- * - Squashes sequences of '/'.
+ * - Squashes sequences of '/' except "//server/share" on Windows
* - Removes "." components.
* - Removes ".." components, and the components the precede them.
* Returns failure (non-zero) if a ".." component appears as first path
@@ -1014,17 +1014,23 @@ const char *remove_leading_path(const char *in, const char *prefix)
int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
{
char *dst0;
- int i;
-
- for (i = has_dos_drive_prefix(src); i > 0; i--)
- *dst++ = *src++;
- dst0 = dst;
+ int offset;
- if (is_dir_sep(*src)) {
+ /*
+ * Handle initial part of absolute path: "/", "C:/", "\\server\share/".
+ */
+ offset = offset_1st_component(src);
+ if (offset) {
+ /* Convert the trailing separator to '/' on Windows. */
+ memcpy(dst, src, offset - 1);
+ dst += offset - 1;
*dst++ = '/';
- while (is_dir_sep(*src))
- src++;
+ src += offset;
}
+ dst0 = dst;
+
+ while (is_dir_sep(*src))
+ src++;
for (;;) {
char c = *src;
--
2.11.0.79.g263f27a
^ permalink raw reply related
* Re: What's cooking in git.git (Dec 2016, #02; Mon, 12)
From: Brandon Williams @ 2016-12-13 21:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Schindelin
In-Reply-To: <xmqqeg1b39yi.fsf@gitster.mtv.corp.google.com>
On 12/13, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > ok 13 - grep tree and more pathspecs
> >
> > expecting success:
> > git init parent &&
> > test_when_finished "rm -rf parent" &&
> > echo "foobar" >"parent/fi:le" &&
> > git -C parent add "fi:le" &&
> > git -C parent commit -m "add fi:le" &&
> > ...
> > test_cmp expect actual
> >
> > ++ git init parent
> > Initialized empty Git repository in C:/git-sdk-64/usr/src/git/wip3/t/trash
> > directory.t7814-grep-recurse-submodules/parent/.git/
> > ++ test_when_finished 'rm -rf parent'
> > ++ test 0 = 0
> > ++ test_cleanup='{ rm -rf parent
> > } && (exit "$eval_ret"); eval_ret=$?; :'
> > ++ echo foobar
> > ++ git -C parent add fi:le
> > fatal: pathspec 'fi:le' did not match any files
>
> I think !MINGW prereq is missing?
Yes the !MINGW prereq is missing on this one test since the test uses a
filename with a ":" which isn't supported on windows. I have that
change made in my local grep branch but I haven't sent out a reroll of
the series yet due to the underlying race-condition that existed (due to
the way realpath was being calculated). I'll send out a reroll of the
series once the discussion on bw/realpath-wo-chdir has concluded (as
the grep series is now dependent on it).
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v2 04/34] sequencer (rebase -i): implement the 'exec' command
From: Junio C Hamano @ 2016-12-13 21:35 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <54d4e8d3673662d1ec806f3f4a779a17effbdaf2.1481642927.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> +static int do_exec(const char *command_line)
> +{
> + const char *child_argv[] = { NULL, NULL };
> + int dirty, status;
> +
> + fprintf(stderr, "Executing: %s\n", command_line);
> + child_argv[0] = command_line;
> + status = run_command_v_opt(child_argv, RUN_USING_SHELL);
> +
> + /* force re-reading of the cache */
> + if (discard_cache() < 0 || read_cache() < 0)
> + return error(_("could not read index"));
> +
> + dirty = require_clean_work_tree("rebase", NULL, 1, 1);
> +
> + if (status) {
> + warning(_("execution failed: %s\n%s"
> + "You can fix the problem, and then run\n"
> + "\n"
> + " git rebase --continue\n"
> + "\n"),
> + command_line,
> + dirty ? N_("and made changes to the index and/or the "
> + "working tree\n") : "");
> + if (status == 127)
> + /* command not found */
> + status = 1;
> + }
> + else if (dirty) {
> + warning(_("execution succeeded: %s\nbut "
> + "left changes to the index and/or the working tree\n"
> + "Commit or stash your changes, and then run\n"
> + "\n"
> + " git rebase --continue\n"
> + "\n"), command_line);
> + status = 1;
> + }
> +
> + return status;
> +}
OK, this looks like a faithful reproduction of what the scripted
version does inside do_next() helper function.
Please have "else if" on the same line as "}" that closes the
"if (...) {" in the same if/else if/else cascade.
^ permalink raw reply
* Re: [PATCH 1/4] doc: add articles (grammar)
From: Kristoffer Haugsbakk @ 2016-12-13 21:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kristoffer Haugsbakk, Philip Oakley, git
In-Reply-To: <xmqqoa0f4s2v.fsf@gitster.mtv.corp.google.com>
Junio C Hamano writes:
> I was planning to merge all four from you as-is to 'next' today,
> though. Should I wait?
I'll definitely defer to whatever you think is best. I guess it depends
on whether you are interested in Philip Oakley's suggestions. I sent
those emails to inform about what I intended to do in the next round, if
it got to that point, since I haven't tried to contribute to such an
organised project before. So I was informing about my assumptions about
how to deal with "looks good to me"-kinds of feedback.
So for my part, I'm happy with iterating on this (perhaps just adding
the two "acks", or also replying to the suggestions), or just merging it
as-is.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v2 06/34] sequencer (rebase -i): write the 'done' file
From: Junio C Hamano @ 2016-12-13 21:52 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <4a1229e9f2d3715607935519f359b5d7986c2290.1481642927.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> In the interactive rebase, commands that were successfully processed are
> not simply discarded, but appended to the 'done' file instead. This is
> used e.g. to display the current state to the user in the output of
> `git status` or the progress.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
Looks good.
I'd stop at this point for now, and start working on other things
for the rest of the day. I might find time to come back to it later
tonight.
So far, looking mostly good.
^ permalink raw reply
* Re: [PATCH v2 02/34] sequencer (rebase -i): implement the 'noop' command
From: Junio C Hamano @ 2016-12-13 21:54 UTC (permalink / raw)
To: Linus Torvalds
Cc: Johannes Schindelin, Git Mailing List, Kevin Daudt,
Dennis Kaarsemaker
In-Reply-To: <CA+55aFzxFFNY+dL6s7dLZeVXBsBKD0aeof5Bj2wcD1CpefVSAA@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Tue, Dec 13, 2016 at 12:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>
>>> +/*
>>> + * Note that ordering matters in this enum. Not only must it match the mapping
>>> + * below, it is also divided into several sections that matter. When adding
>>> + * new commands, make sure you add it in the right section.
>>> + */
>>
>> Good thinking. Makes me wish C were a better language, though ;-)
>
> Do this:
>
> static const char *todo_command_strings[] = {
> [TODO_PICK] = "pick",
> [TODO_REVERT] = "revert",
> [TODO_NOOP] = "noop:,
> };
>
> which makes the array be order-independent. You still need to make
> sure you fill in all the entries, of course, but it tends to avoid at
> least one gotcha, and it makes it more obvious how the two are tied
> together.
>
> Linus
Yes, I know. But I do not think the variant of C we stick to is not
new enough to have that.
^ permalink raw reply
* Re: [PATCH v2 05/34] sequencer (rebase -i): learn about the 'verbose' mode
From: Junio C Hamano @ 2016-12-13 21:51 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <1d1f8d8b0696769bb85dd8a2269dc281aa91eede.1481642927.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> @@ -1493,9 +1498,26 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
> }
>
> if (is_rebase_i(opts)) {
> + struct strbuf buf = STRBUF_INIT;
> +
> /* Stopped in the middle, as planned? */
> if (todo_list->current < todo_list->nr)
> return 0;
> +
> + if (opts->verbose) {
> + const char *argv[] = {
> + "diff-tree", "--stat", NULL, NULL
> + };
> +
> + if (!read_oneliner(&buf, rebase_path_orig_head(), 0))
> + return error(_("could not read '%s'"),
> + rebase_path_orig_head());
> + strbuf_addstr(&buf, "..HEAD");
> + argv[2] = buf.buf;
> + run_command_v_opt(argv, RUN_GIT_CMD);
> + strbuf_reset(&buf);
> + }
> + strbuf_release(&buf);
> }
It's a bit curious that the previous step avoided running a separate
process and instead did "diff-tree -p" all in C, but this one does not.
I think it is because this one is outside the loop? The original,
being a scripted Porcelain, formulates a lazy and loose command
line, but you may want to tighten it up a bit if you spawn a
process. If your user happens to have a file whose name is
$orig_head..HEAD, the command line you are creating (which is
identical to the scripted version) will barf with "ambiguous
argument".
One good thing about a complete C rewrite is that it won't have an
issue like this one because you'd be working with in-core objects.
> diff --git a/sequencer.h b/sequencer.h
> index cb21cfddee..f885b68395 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -24,6 +24,7 @@ struct replay_opts {
> int allow_empty;
> int allow_empty_message;
> int keep_redundant_commits;
> + int verbose;
>
> int mainline;
^ permalink raw reply
* Re: [PATCH] fix pushing to //server/share/dir paths on Windows
From: Junio C Hamano @ 2016-12-13 22:48 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, Jeff King, Git Mailing List
In-Reply-To: <2ff2613c-47da-a780-5d38-93e16cb16328@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> There is a change in behavior: \\server\share is not transformed
> into //server/share anymore, but all subsequent directory separators
> are rewritten to '/'. This should not make a difference; Windows can
> handle the mix.
I saw Dscho had a similar "windows can handle the mix" change in an
earlier development cycle, I think, and this is being consistent.
> Another long-standing bug uncovered by the quarantine series.
>
> Dscho, it looks like this could fix the original report at
> https://github.com/git-for-windows/git/issues/979
>
> This patch should cook well because of the change in behavior.
> I would not be surprised if there is some fall-out.
>
> The other bug I'm alluding to, I still have to investigate. I do
> not think that it can be counted as fall-out.
>
> path.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
Thanks.
> diff --git a/path.c b/path.c
> index 52d889c88e..02dc70fb92 100644
> --- a/path.c
> +++ b/path.c
> @@ -991,7 +991,7 @@ const char *remove_leading_path(const char *in, const char *prefix)
> *
> * Performs the following normalizations on src, storing the result in dst:
> * - Ensures that components are separated by '/' (Windows only)
> - * - Squashes sequences of '/'.
> + * - Squashes sequences of '/' except "//server/share" on Windows
"on windows" because offset_1st_component() does the magic only
there? Makes sense.
> * - Removes "." components.
> * - Removes ".." components, and the components the precede them.
> * Returns failure (non-zero) if a ".." component appears as first path
> @@ -1014,17 +1014,23 @@ const char *remove_leading_path(const char *in, const char *prefix)
> int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
> {
> char *dst0;
> - int i;
> -
> - for (i = has_dos_drive_prefix(src); i > 0; i--)
> - *dst++ = *src++;
> - dst0 = dst;
> + int offset;
>
> - if (is_dir_sep(*src)) {
> + /*
> + * Handle initial part of absolute path: "/", "C:/", "\\server\share/".
> + */
> + offset = offset_1st_component(src);
> + if (offset) {
> + /* Convert the trailing separator to '/' on Windows. */
> + memcpy(dst, src, offset - 1);
> + dst += offset - 1;
> *dst++ = '/';
> - while (is_dir_sep(*src))
> - src++;
> + src += offset;
> }
> + dst0 = dst;
By resetting dst0 here, we ensure that up_one that is triggered by
seeing "../" will not escape the \\server\share\ part, which makes
sense to me.
> + while (is_dir_sep(*src))
> + src++;
>
> for (;;) {
> char c = *src;
^ permalink raw reply
* Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface
From: Brandon Williams @ 2016-12-13 22:49 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <20161209192316.GB88637@google.com>
On 12/09, Brandon Williams wrote:
> On 12/09, Duy Nguyen wrote:
> > On Fri, Dec 9, 2016 at 1:19 AM, Brandon Williams <bmwill@google.com> wrote:
> > > On 12/08, Duy Nguyen wrote:
> > >> On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams <bmwill@google.com> wrote:
> > >> > On 12/07, Duy Nguyen wrote:
> > >> >> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:
> > >> >> > Convert 'create_simplify()' to use the pathspec struct interface from
> > >> >> > using the '_raw' entry in the pathspec.
> > >> >>
> > >> >> It would be even better to kill this create_simplify() and let
> > >> >> simplify_away() handle struct pathspec directly.
> > >> >>
> > >> >> There is a bug in this code, that might have been found if we
> > >> >> simpify_away() handled pathspec directly: the memcmp() in
> > >> >> simplify_away() will not play well with :(icase) magic. My bad. If
> > >> >> :(icase) is used, the easiest/safe way is simplify nothing. Later on
> > >> >> maybe we can teach simplify_away() to do strncasecmp instead. We could
> > >> >> ignore exclude patterns there too (although not excluding is not a
> > >> >> bug).
> > >> >
> > >> > So are you implying that the simplify struct needs to be killed? That
> > >> > way the pathspec struct itself is being passed around instead?
> > >>
> > >> Yes. simplify struct was a thing when pathspec was an array of char *.
> > >> At this point I think it can retire (when we have time to retire it)
> > >
> > > Alright, then for now I can leave this change as is and have a follow up
> > > series that kills the simplify struct.
> >
> > Do let me know if you decide to drop it, so I can put it back in my backlog.
>
> K will do
>
This actually turned out to be more straight forward than I thought.
I'll reroll this series again (with a few other changes) and include
killing the simplify struct.
--
Brandon Williams
^ permalink raw reply
* [PATCHv3] git-p4: support git worktrees
From: Luke Diamand @ 2016-12-13 21:51 UTC (permalink / raw)
To: git
Cc: viniciusalexandre, Lars Schneider, Duy Nguyen, Junio C Hamano,
Luke Diamand
In-Reply-To: <20161213215128.20288-1-luke@diamand.org>
git-p4 would attempt to find the git directory using
its own specific code, which did not know about git
worktrees.
Rework it to use "git rev-parse --git-dir" instead.
Add test cases for worktree usage and specifying
git directory via --git-dir and $GIT_DIR.
Signed-off-by: Luke Diamand <luke@diamand.org>
---
git-p4.py | 17 +++++++++++++----
t/t9800-git-p4-basic.sh | 20 ++++++++++++++++++++
t/t9806-git-p4-options.sh | 32 ++++++++++++++++++++++++++++++++
3 files changed, 65 insertions(+), 4 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index fd5ca52..6a1f65f 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -85,6 +85,16 @@ def p4_build_cmd(cmd):
real_cmd += cmd
return real_cmd
+def git_dir(path):
+ """ Return TRUE if the given path is a git directory (/path/to/dir/.git).
+ This won't automatically add ".git" to a directory.
+ """
+ d = read_pipe(["git", "--git-dir", path, "rev-parse", "--git-dir"], True).strip()
+ if not d or len(d) == 0:
+ return None
+ else:
+ return d
+
def chdir(path, is_client_path=False):
"""Do chdir to the given path, and set the PWD environment
variable for use by P4. It does not look at getcwd() output.
@@ -563,10 +573,7 @@ def currentGitBranch():
return read_pipe(["git", "name-rev", "HEAD"]).split(" ")[1].strip()
def isValidGitDir(path):
- if (os.path.exists(path + "/HEAD")
- and os.path.exists(path + "/refs") and os.path.exists(path + "/objects")):
- return True;
- return False
+ return git_dir(path) != None
def parseRevision(ref):
return read_pipe("git rev-parse %s" % ref).strip()
@@ -3682,6 +3689,7 @@ def main():
if cmd.gitdir == None:
cmd.gitdir = os.path.abspath(".git")
if not isValidGitDir(cmd.gitdir):
+ # "rev-parse --git-dir" without arguments will try $PWD/.git
cmd.gitdir = read_pipe("git rev-parse --git-dir").strip()
if os.path.exists(cmd.gitdir):
cdup = read_pipe("git rev-parse --show-cdup").strip()
@@ -3694,6 +3702,7 @@ def main():
else:
die("fatal: cannot locate git repository at %s" % cmd.gitdir)
+ # so git commands invoked from the P4 workspace will succeed
os.environ["GIT_DIR"] = cmd.gitdir
if not cmd.run(args):
diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
index 0730f18..093e9bd 100755
--- a/t/t9800-git-p4-basic.sh
+++ b/t/t9800-git-p4-basic.sh
@@ -257,6 +257,26 @@ test_expect_success 'submit from detached head' '
)
'
+test_expect_success 'submit from worktree' '
+ test_when_finished cleanup_git &&
+ git p4 clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git worktree add ../worktree-test
+ ) &&
+ (
+ cd "$git/../worktree-test" &&
+ test_commit "worktree-commit" &&
+ git config git-p4.skipSubmitEdit true &&
+ git p4 submit
+ ) &&
+ (
+ cd "$cli" &&
+ p4 sync &&
+ test_path_is_file worktree-commit.t
+ )
+'
+
test_expect_success 'kill p4d' '
kill_p4d
'
diff --git a/t/t9806-git-p4-options.sh b/t/t9806-git-p4-options.sh
index 254d428..1ab76c4 100755
--- a/t/t9806-git-p4-options.sh
+++ b/t/t9806-git-p4-options.sh
@@ -269,6 +269,38 @@ test_expect_success 'submit works with two branches' '
)
'
+test_expect_success 'use --git-dir option and GIT_DIR' '
+ test_when_finished cleanup_git &&
+ git p4 clone //depot --destination="$git" &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ test_commit first-change &&
+ git p4 submit --git-dir "$git"
+ ) &&
+ (
+ cd "$cli" &&
+ p4 sync &&
+ test_path_is_file first-change.t &&
+ echo "cli_file" >cli_file.t &&
+ p4 add cli_file.t &&
+ p4 submit -d "cli change"
+ ) &&
+ (git --git-dir "$git" p4 sync) &&
+ (cd "$git" && git checkout -q p4/master) &&
+ test_path_is_file "$git"/cli_file.t &&
+ (
+ cd "$cli" &&
+ echo "cli_file2" >cli_file2.t &&
+ p4 add cli_file2.t &&
+ p4 submit -d "cli change2"
+ ) &&
+ (GIT_DIR="$git" git p4 sync) &&
+ (cd "$git" && git checkout -q p4/master) &&
+ test_path_is_file "$git"/cli_file2.t
+'
+
+
test_expect_success 'kill p4d' '
kill_p4d
'
--
2.8.2.703.g78b384c.dirty
^ 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