* Re: Allow "git shortlog" to group by committer information
From: Jacob Keller @ 2016-12-21 7:55 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Johannes Sixt, Linus Torvalds, Git Mailing List
In-Reply-To: <20161221032221.s7jmgnfrr6tyuyuk@sigill.intra.peff.net>
On Tue, Dec 20, 2016 at 7:22 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Dec 20, 2016 at 10:35:36AM -0800, Junio C Hamano wrote:
>
>> -- >8 --
>> Subject: SQUASH???
>>
>> Make sure the test does not depend on the result of the previous
>> tests; with MINGW prerequisite satisfied, a "reset to original and
>> rebuild" in an earlier test was skipped, resulting in different
>> history being tested with this and the next tests.
>
> Yeah, this looks good, and obviously correct.
>
> I do wonder if in general it should be the responsibility of skippable
> tests to make sure we end up with the same state whether they are run or
> not. That might manage the complexity more. But I certainly don't mind
> tests being defensive like you have here.
>
> -Peff
That seems like a good idea, but I'm not sure how you would implement
it in practice? Would we just "rely" on a skipable test having a "do
this if we skip, instead" block? That would be easier to spot but I
think still relies on the skip-able tests being careful?
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH] mailinfo.c: move side-effects outside of assert
From: Kyle J. McKay @ 2016-12-21 5:54 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin, Jonathan Tan, Junio C Hamano,
Git mailing list
In-Reply-To: <20161220164526.qnwnmr7cvyycmw6a@sigill.intra.peff.net>
On Dec 20, 2016, at 08:45, Jeff King wrote:
> On Tue, Dec 20, 2016 at 03:12:35PM +0100, Johannes Schindelin wrote:
>
>>> On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:
>>>
>>>> ACK. I noticed this problem (and fixed it independently as a part
>>>> of a
>>>> huge patch series I did not get around to submit yet) while
>>>> trying to
>>>> get Git to build correctly with Visual C.
>>>
>>> Does this mean that Dscho and I are the only ones who add -DNDEBUG
>>> for
>>> release builds? Or are we just the only ones who actually run the
>>> test
>>> suite on such builds?
>>
>> It seems you and I are for the moment the only ones bothering with
>> running
>> the test suite on release builds.
>
> I wasn't aware anybody actually built with NDEBUG at all. You'd have
> to
> explicitly ask for it via CFLAGS, so I assume most people don't.
Not a good assumption. You know what happens when you assume[1],
right? ;)
I've been defining NDEBUG whenever I make a release build for quite
some time (not just for Git) in order to squeeze every last possible
drop of performance out of it.
> Certainly I never have when deploying to GitHub's cluster (let alone
> my
> personal use), and I note that the Debian package also does not.
Yeah, I don't do it for my personal use because those are often not
based on a release tag so I want to see any assertion failures that
might happen and they're also not performance critical either.
> So from my perspective it is not so much "do not bother with release
> builds" as "are release builds even a thing for git?"
They should be if you're deploying Git in a performance critical
environment.
> One of the
> reasons I suggested switching the assert() to a die("BUG") is that the
> latter cannot be disabled. We generally seem to prefer those to
> assert()
> in our code-base (though there is certainly a mix). If the assertions
> are not expensive to compute, I think it is better to keep them in for
> all builds. I'd much rather get a report from a user that says "I hit
> this BUG" than "git segfaulted and I have no idea where" (of course I
> prefer a backtrace even more, but that's not always an option).
Perhaps Git should provide a "verify" macro. Works like "assert"
except that it doesn't go away when NDEBUG is defined. Being Git-
provided it could also use Git's die function. Then Git could do a
global replace of assert with verify and institute a no-assert policy.
> I do notice that we set NDEBUG for nedmalloc, though if I am reading
> the
> Makefile right, it is just for compiling those files. It looks like
> there are a ton of asserts there that _are_ potentially expensive, so
> that makes sense.
So there's no way to get a non-release build of nedmalloc inside Git
then without hacking the Makefile? What if you need those assertions
enabled? Maybe NDEBUG shouldn't be defined by default for any files.
--Kyle
[1] https://www.youtube.com/watch?v=KEP1acj29-Y
^ permalink raw reply
* Re: Allow "git shortlog" to group by committer information
From: Jeff King @ 2016-12-21 3:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Linus Torvalds, Git Mailing List
In-Reply-To: <xmqqvauejvnr.fsf@gitster.mtv.corp.google.com>
On Tue, Dec 20, 2016 at 10:35:36AM -0800, Junio C Hamano wrote:
> -- >8 --
> Subject: SQUASH???
>
> Make sure the test does not depend on the result of the previous
> tests; with MINGW prerequisite satisfied, a "reset to original and
> rebuild" in an earlier test was skipped, resulting in different
> history being tested with this and the next tests.
Yeah, this looks good, and obviously correct.
I do wonder if in general it should be the responsibility of skippable
tests to make sure we end up with the same state whether they are run or
not. That might manage the complexity more. But I certainly don't mind
tests being defensive like you have here.
-Peff
^ permalink raw reply
* config for `format-patch --notes`?
From: Norbert Kiesel @ 2016-12-21 0:18 UTC (permalink / raw)
To: git
Hi,
I use `git format-patch master..myBranch` quite a bit to send patches
to other developers. I also add notes to the commits
so that I e.g. remember which patches were emailed to whom. `git log`
has an option to automatically include the notes in
the output. However, I can't find such an option for `git
format-patch`. Am I missing something?
Another nice option would to to somehow include the branch name in the
resulting output. Right now I use either notes
or abuse the `--subject` option for this.
</nk>
P.S.: Today I'm sad and proud to say "Ich bin ein Berliner!" --nk
^ permalink raw reply
* Re: [PATCH 00/13] gitk: tweak rendering of remote-tracking references
From: Michael Haggerty @ 2016-12-21 0:05 UTC (permalink / raw)
To: Marc Branchaud, Paul Mackerras; +Cc: git
In-Reply-To: <97d97bc6-54f1-2ef2-fe04-7e7f144d7e51@xiplink.com>
On 12/20/2016 04:01 PM, Marc Branchaud wrote:
> On 2016-12-19 11:44 AM, Michael Haggerty wrote:
>> This patch series changes a bunch of details about how remote-tracking
>> references are rendered in the commit list of gitk:
>
> Thanks for this! I like the new, compact look very much!
>
> That said, I remember when I was a new git user and I leaned heavily on
> gitk to understand how references worked. It was particularly
> illuminating to see the remote references distinctly labeled, and the
> fact that they were "remotes/origin/foo" gave me an Aha! moment where I
> came to understand that the refs hierarchy is more flexible than just
> the conventions coded into git itself. I eventually felt free to create
> my own, private ref hierarchies.
>
> I am in no way opposed to this series. I just wanted to point out that
> there was some utility in those labels. It makes me think that it might
> be worthwhile for gitk to have a "raw-refs" mode, that shows the full
> "refs/foo/bar/baz" paths of all the heads, tags, and whatever else. It
> could be a useful teaching tool for git.
Yes, I understand that the longer names might be useful for beginners,
and the full names even more so. However, I think once a user has that
"aha!" moment, the space wasted on all the redundant words is a real
impediment to gitk's usability. It is common to have a few references on
a single commit (especially if you pull from multiple remotes), in which
case the summary line is completely invisible (and therefore its context
menu is unreachable). I don't like the idea of dumbing down the UI
permanently based on what users need at the very beginning of their Git
usage.
Would it be possible to use the short names in the UI but to add the
full names to a tooltip or to the context menu?
>> * Omit the "remote/" prefix on normal remote-tracking references. They
>
> If you re-roll, s:remote/:remotes/:.
Thanks.
>> are already distinguished via their two-tone rendering and (usually)
>> longer names, and this change saves a lot of visual clutter and
>> horizontal space.
>>
>> * Render remote-tracking references that have more than the usual
>> three slashes like
>>
>> origin/foo/bar
>> ^^^^^^^
>>
>> rather than
>>
>> origin/foo/bar (formerly remotes/origin/foo/bar)
>> ^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^
>>
>> , where the indicated part is the prefix that is rendered in a
>> different color. Usually, such a reference represents a remote
>> branch that contains a slash in its name, so the new split more
>> accurately portrays the separation between remote name and remote
>> branch name.
>
> *Love* this change! :)
>
>> * Introduce a separate constant to specify the background color used
>> for the branch name part of remote-tracking references, to allow it
>> to differ from the color used for local branches (which by default
>> is bright green).
>>
>> * Change the default background colors for remote-tracking branches to
>> light brown and brown (formerly they were pale orange and bright
>> green).
>
> Please don't change the remotebgcolor default.
>
> Also, perhaps the default remoterefbgcolor should be
> set remoterefbgcolor $headbgcolor
> ?
>
> I say this because when I applied the series, without the last patch, I
> was miffed that the remote/ref colour had changed.
This is a one-time inconvenience that gitk developers will experience. I
doubt that users jump backwards and forwards in gitk versions very often.
I do find it strange that gitk writes a color selection to its
configuration file *even if the user has left it at its default*.
Normally I would expect only user-changed settings to be written to the
config file, and other values to use the default that is built into the
program. For example, such an approach would have made the transition
from "green" to "lime" easier.
> [...]
Thanks for your feedback!
Michael
^ permalink raw reply
* Re: [PATCH v2 09/34] sequencer (rebase -i): write an author-script file
From: Junio C Hamano @ 2016-12-20 23:46 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <xmqqy3zbl718.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
>> We keep coming back to the same argument. You want this quoting/dequoting
>> to be turned into a full-fledged parser. And I keep pointing out that the
>> code here does not *need* to parse but only construct an environment
>> block.
>
> I am afraid you are mis-reading me. I see a code that _READS_ some
> data format, for which we already have a parser, and then write out
> things based on what it read. I do not want you to make anything
> into a full-fledged parser---I just do not want to see an ad-hoc
> reader and instead the code to USE existing parser.
To extend this a bit.
I care rather deeply about not spreading the re-invention of parsers
and formatters throughout the code, because that would introduce
unnecesary maintenance burden.
Quoting a string so that it is acceptable inside a pair of single
quotes, occasionally stepping outside of the sq context and using
backquote to excape individual byte, for example, might feel simple
enough that anybody can just write inline instead of learning how to
call and actually calling sq_quote(). You open ', show each byte
unless it is a ', in which case you close ' and give \' and
immediately open '. That was what sq_quote() did originally and for
a long time. If however we allowed everybody to reinvent the code
to quote all over the place, with a lame "This is different and does
not *need* to call shared code" excuse, it would have required a lot
more effort to do a change like the one in 77d604c309 ("Enhanced
sq_quote()", 2005-10-10) that changed the quoting rules slightly to
make the output safer to accomodate different variants of shells.
The same thing can be said for split_ident() code. The "author"
line has name, '<', email address, '>', timestring, '+' or '-', and
timezone. Splitting them into NAME and TIME may be very simple, and
"does not *need* to parse", right? Not really. If you look at how
split_ident() evolved to accomodate the real world malformed lines
like duplicated closing '>' and realize that what we have there may
probably *NOT* the perfect one (i.e. there may be other malformed
input we may have to accomodate by tweaking the implementation
further), we really do not want a hand-rolled ad-hoc "splitter" that
"does not *need* to parse".
The same thing can be said for the code that reads the
author-script, too.
So please do not waste any more time arguing. I think you spent
arguing more time than you would otherwise have had to spend if you
just used existing helper functions, both in this thread and the
older one about reading the author-script.
^ permalink raw reply
* [PATCHv4 3/4] submodule: rename and add flags to ok_to_remove_submodule
From: Stefan Beller @ 2016-12-20 23:12 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
In-Reply-To: <20161220231227.14115-1-sbeller@google.com>
In different contexts the question "Is it ok to delete a submodule?"
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.
As the flags allow the function to not die on an error when spawning
a child process, we need to find an appropriate return code for the
case when the child process could not be started. As in that case we
cannot tell if the submodule is ok to remove, we'd want to return 'false'.
As only 0 is understood as false, rename the function to invert the
meaning, i.e. the return code of 0 signals the removal of the submodule
is fine, and other values can be used to return a more precise answer
what went wrong.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
builtin/rm.c | 4 +++-
submodule.c | 49 +++++++++++++++++++++++++++++++++++++------------
submodule.h | 6 +++++-
3 files changed, 45 insertions(+), 14 deletions(-)
diff --git a/builtin/rm.c b/builtin/rm.c
index 3f3e24eb36..5a5a66272b 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -187,7 +187,9 @@ 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)))
+ bad_to_remove_submodule(ce->name,
+ SUBMODULE_REMOVAL_DIE_ON_ERROR |
+ SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED)))
local_changes = 1;
/*
diff --git a/submodule.c b/submodule.c
index 9f0b544ebe..1cc04d24e5 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1019,39 +1019,64 @@ int submodule_uses_gitfile(const char *path)
return 1;
}
-int ok_to_remove_submodule(const char *path)
+/*
+ * Check if it is a bad idea to remove a submodule, i.e. if we'd lose data
+ * when doing so.
+ *
+ * Return 1 if we'd lose data, return 0 if the removal is fine,
+ * and negative values for errors.
+ */
+int bad_to_remove_submodule(const char *path, unsigned flags)
{
ssize_t len;
struct child_process cp = CHILD_PROCESS_INIT;
struct strbuf buf = STRBUF_INIT;
- int ok_to_remove = 1;
+ int ret = 0;
if (!file_exists(path) || is_empty_dir(path))
- return 1;
+ return 0;
if (!submodule_uses_gitfile(path))
- return 0;
+ return 1;
- 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);
+ if (start_command(&cp)) {
+ if (flags & SUBMODULE_REMOVAL_DIE_ON_ERROR)
+ die(_("could not start 'git status in submodule '%s'"),
+ path);
+ ret = -1;
+ goto out;
+ }
len = strbuf_read(&buf, cp.out, 1024);
if (len > 2)
- ok_to_remove = 0;
+ ret = 1;
close(cp.out);
- if (finish_command(&cp))
- die(_("'git status --porcelain -u --ignore-submodules=none' failed in submodule %s"), path);
-
+ if (finish_command(&cp)) {
+ if (flags & SUBMODULE_REMOVAL_DIE_ON_ERROR)
+ die(_("could not run 'git status in submodule '%s'"),
+ path);
+ ret = -1;
+ }
+out:
strbuf_release(&buf);
- return ok_to_remove;
+ return ret;
}
static int find_first_merges(struct object_array *result, const char *path,
diff --git a/submodule.h b/submodule.h
index 61fb610749..21b1569413 100644
--- a/submodule.h
+++ b/submodule.h
@@ -59,7 +59,11 @@ 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_DIE_ON_ERROR (1<<0)
+#define SUBMODULE_REMOVAL_IGNORE_UNTRACKED (1<<1)
+#define SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED (1<<2)
+extern int bad_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.53.gb7b3fba.dirty
^ permalink raw reply related
* [PATCHv4 1/4] submodule.h: add extern keyword to functions
From: Stefan Beller @ 2016-12-20 23:12 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
In-Reply-To: <20161220231227.14115-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.53.gb7b3fba.dirty
^ permalink raw reply related
* [PATCHv4 4/4] rm: absorb a submodules git dir before deletion
From: Stefan Beller @ 2016-12-20 23:12 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
In-Reply-To: <20161220231227.14115-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.
Now that the functionality is available to absorb the git directory of a
submodule, rewrite the checking in git-rm to not complain, but rather
relocate the git directories inside the superproject.
An alternative solution was discussed to have a function
`depopulate_submodule`. That would couple the check for its git directory
and possible relocation before the the removal, such that it is less
likely to miss the check in the future. But the indirection with such
a function added seemed also complex. The reason for that was that this
possible move of the git directory was also implemented in
`ok_to_remove_submodule`, such that this function could truthfully
answer whether it is ok to remove the submodule.
The solution proposed here defers all these checks to the caller.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
builtin/rm.c | 79 ++++++++++++++---------------------------------------------
t/t3600-rm.sh | 39 ++++++++++++-----------------
2 files changed, 33 insertions(+), 85 deletions(-)
diff --git a/builtin/rm.c b/builtin/rm.c
index 5a5a66272b..6f2001b0eb 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -59,27 +59,9 @@ static void print_error_files(struct string_list *files_list,
}
}
-static void error_removing_concrete_submodules(struct string_list *files, int *errs)
-{
- print_error_files(files,
- Q_("the following submodule (or one of its nested "
- "submodules)\n"
- "uses a .git directory:",
- "the following submodules (or one of their nested "
- "submodules)\n"
- "use a .git directory:", files->nr),
- _("\n(use 'rm -rf' if you really want to remove "
- "it including all of its history)"),
- errs);
- string_list_clear(files, 0);
-}
-
-static int check_submodules_use_gitfiles(void)
+static void submodules_absorb_gitdir_if_needed(const char *prefix)
{
int i;
- int errs = 0;
- struct string_list files = STRING_LIST_INIT_NODUP;
-
for (i = 0; i < list.nr; i++) {
const char *name = list.entry[i].name;
int pos;
@@ -99,12 +81,9 @@ static int check_submodules_use_gitfiles(void)
continue;
if (!submodule_uses_gitfile(name))
- string_list_append(&files, name);
+ absorb_git_dir_into_superproject(prefix, name,
+ ABSORB_GITDIR_RECURSE_SUBMODULES);
}
-
- error_removing_concrete_submodules(&files, &errs);
-
- return errs;
}
static int check_local_mod(struct object_id *head, int index_only)
@@ -120,7 +99,6 @@ static int check_local_mod(struct object_id *head, int index_only)
int errs = 0;
struct string_list files_staged = STRING_LIST_INIT_NODUP;
struct string_list files_cached = STRING_LIST_INIT_NODUP;
- struct string_list files_submodule = STRING_LIST_INIT_NODUP;
struct string_list files_local = STRING_LIST_INIT_NODUP;
no_head = is_null_oid(head);
@@ -219,13 +197,8 @@ static int check_local_mod(struct object_id *head, int index_only)
else if (!index_only) {
if (staged_changes)
string_list_append(&files_cached, name);
- if (local_changes) {
- if (S_ISGITLINK(ce->ce_mode) &&
- !submodule_uses_gitfile(name))
- string_list_append(&files_submodule, name);
- else
- string_list_append(&files_local, name);
- }
+ if (local_changes)
+ string_list_append(&files_local, name);
}
}
print_error_files(&files_staged,
@@ -247,8 +220,6 @@ static int check_local_mod(struct object_id *head, int index_only)
&errs);
string_list_clear(&files_cached, 0);
- error_removing_concrete_submodules(&files_submodule, &errs);
-
print_error_files(&files_local,
Q_("the following file has local modifications:",
"the following files have local modifications:",
@@ -342,6 +313,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
exit(0);
}
+ submodules_absorb_gitdir_if_needed(prefix);
+
/*
* If not forced, the file, the index and the HEAD (if exists)
* must match; but the file can already been removed, since
@@ -358,9 +331,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
oidclr(&oid);
if (check_local_mod(&oid, index_only))
exit(1);
- } else if (!index_only) {
- if (check_submodules_use_gitfiles())
- exit(1);
}
/*
@@ -389,32 +359,20 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
*/
if (!index_only) {
int removed = 0, gitmodules_modified = 0;
- struct strbuf buf = STRBUF_INIT;
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. */
- }
+ struct strbuf buf = STRBUF_INIT;
+
+ strbuf_addstr(&buf, path);
+ if (remove_dir_recursively(&buf, 0))
+ die(_("could not remove '%s'"), path);
+ strbuf_release(&buf);
+
+ removed = 1;
+ if (!remove_path_from_gitmodules(path))
+ gitmodules_modified = 1;
+ continue;
}
if (!remove_path(path)) {
removed = 1;
@@ -423,7 +381,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
if (!removed)
die_errno("git rm: '%s'", path);
}
- strbuf_release(&buf);
if (gitmodules_modified)
stage_updated_gitmodules();
}
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.53.gb7b3fba.dirty
^ permalink raw reply related
* [PATCHv4 2/4] submodule: modernize ok_to_remove_submodule to use argv_array
From: Stefan Beller @ 2016-12-20 23:12 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
In-Reply-To: <20161220231227.14115-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.53.gb7b3fba.dirty
^ permalink raw reply related
* [PATCHv5 4/4] rm: absorb a submodules git dir before deletion
From: Stefan Beller @ 2016-12-20 23:20 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, David.Turner, sandals, j6t, Stefan Beller
In-Reply-To: <20161220232012.15997-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.
Now that the functionality is available to absorb the git directory of a
submodule, rewrite the checking in git-rm to not complain, but rather
relocate the git directories inside the superproject.
An alternative solution was discussed to have a function
`depopulate_submodule`. That would couple the check for its git directory
and possible relocation before the the removal, such that it is less
likely to miss the check in the future. But the indirection with such
a function added seemed also complex. The reason for that was that this
possible move of the git directory was also implemented in
`ok_to_remove_submodule`, such that this function could truthfully
answer whether it is ok to remove the submodule.
The solution proposed here defers all these checks to the caller.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
builtin/rm.c | 79 ++++++++++++++---------------------------------------------
t/t3600-rm.sh | 39 ++++++++++++-----------------
2 files changed, 33 insertions(+), 85 deletions(-)
diff --git a/builtin/rm.c b/builtin/rm.c
index 5a5a66272b..6f2001b0eb 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -59,27 +59,9 @@ static void print_error_files(struct string_list *files_list,
}
}
-static void error_removing_concrete_submodules(struct string_list *files, int *errs)
-{
- print_error_files(files,
- Q_("the following submodule (or one of its nested "
- "submodules)\n"
- "uses a .git directory:",
- "the following submodules (or one of their nested "
- "submodules)\n"
- "use a .git directory:", files->nr),
- _("\n(use 'rm -rf' if you really want to remove "
- "it including all of its history)"),
- errs);
- string_list_clear(files, 0);
-}
-
-static int check_submodules_use_gitfiles(void)
+static void submodules_absorb_gitdir_if_needed(const char *prefix)
{
int i;
- int errs = 0;
- struct string_list files = STRING_LIST_INIT_NODUP;
-
for (i = 0; i < list.nr; i++) {
const char *name = list.entry[i].name;
int pos;
@@ -99,12 +81,9 @@ static int check_submodules_use_gitfiles(void)
continue;
if (!submodule_uses_gitfile(name))
- string_list_append(&files, name);
+ absorb_git_dir_into_superproject(prefix, name,
+ ABSORB_GITDIR_RECURSE_SUBMODULES);
}
-
- error_removing_concrete_submodules(&files, &errs);
-
- return errs;
}
static int check_local_mod(struct object_id *head, int index_only)
@@ -120,7 +99,6 @@ static int check_local_mod(struct object_id *head, int index_only)
int errs = 0;
struct string_list files_staged = STRING_LIST_INIT_NODUP;
struct string_list files_cached = STRING_LIST_INIT_NODUP;
- struct string_list files_submodule = STRING_LIST_INIT_NODUP;
struct string_list files_local = STRING_LIST_INIT_NODUP;
no_head = is_null_oid(head);
@@ -219,13 +197,8 @@ static int check_local_mod(struct object_id *head, int index_only)
else if (!index_only) {
if (staged_changes)
string_list_append(&files_cached, name);
- if (local_changes) {
- if (S_ISGITLINK(ce->ce_mode) &&
- !submodule_uses_gitfile(name))
- string_list_append(&files_submodule, name);
- else
- string_list_append(&files_local, name);
- }
+ if (local_changes)
+ string_list_append(&files_local, name);
}
}
print_error_files(&files_staged,
@@ -247,8 +220,6 @@ static int check_local_mod(struct object_id *head, int index_only)
&errs);
string_list_clear(&files_cached, 0);
- error_removing_concrete_submodules(&files_submodule, &errs);
-
print_error_files(&files_local,
Q_("the following file has local modifications:",
"the following files have local modifications:",
@@ -342,6 +313,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
exit(0);
}
+ submodules_absorb_gitdir_if_needed(prefix);
+
/*
* If not forced, the file, the index and the HEAD (if exists)
* must match; but the file can already been removed, since
@@ -358,9 +331,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
oidclr(&oid);
if (check_local_mod(&oid, index_only))
exit(1);
- } else if (!index_only) {
- if (check_submodules_use_gitfiles())
- exit(1);
}
/*
@@ -389,32 +359,20 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
*/
if (!index_only) {
int removed = 0, gitmodules_modified = 0;
- struct strbuf buf = STRBUF_INIT;
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. */
- }
+ struct strbuf buf = STRBUF_INIT;
+
+ strbuf_addstr(&buf, path);
+ if (remove_dir_recursively(&buf, 0))
+ die(_("could not remove '%s'"), path);
+ strbuf_release(&buf);
+
+ removed = 1;
+ if (!remove_path_from_gitmodules(path))
+ gitmodules_modified = 1;
+ continue;
}
if (!remove_path(path)) {
removed = 1;
@@ -423,7 +381,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
if (!removed)
die_errno("git rm: '%s'", path);
}
- strbuf_release(&buf);
if (gitmodules_modified)
stage_updated_gitmodules();
}
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.53.gb7b3fba.dirty
^ permalink raw reply related
* [PATCHv5 3/4] submodule: rename and add flags to ok_to_remove_submodule
From: Stefan Beller @ 2016-12-20 23:20 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, David.Turner, sandals, j6t, Stefan Beller
In-Reply-To: <20161220232012.15997-1-sbeller@google.com>
In different contexts the question "Is it ok to delete a submodule?"
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.
As the flags allow the function to not die on an error when spawning
a child process, we need to find an appropriate return code for the
case when the child process could not be started. As in that case we
cannot tell if the submodule is ok to remove, we'd want to return 'false'.
As only 0 is understood as false, rename the function to invert the
meaning, i.e. the return code of 0 signals the removal of the submodule
is fine, and other values can be used to return a more precise answer
what went wrong.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
builtin/rm.c | 4 +++-
submodule.c | 49 +++++++++++++++++++++++++++++++++++++------------
submodule.h | 6 +++++-
3 files changed, 45 insertions(+), 14 deletions(-)
diff --git a/builtin/rm.c b/builtin/rm.c
index 3f3e24eb36..5a5a66272b 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -187,7 +187,9 @@ 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)))
+ bad_to_remove_submodule(ce->name,
+ SUBMODULE_REMOVAL_DIE_ON_ERROR |
+ SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED)))
local_changes = 1;
/*
diff --git a/submodule.c b/submodule.c
index 9f0b544ebe..1cc04d24e5 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1019,39 +1019,64 @@ int submodule_uses_gitfile(const char *path)
return 1;
}
-int ok_to_remove_submodule(const char *path)
+/*
+ * Check if it is a bad idea to remove a submodule, i.e. if we'd lose data
+ * when doing so.
+ *
+ * Return 1 if we'd lose data, return 0 if the removal is fine,
+ * and negative values for errors.
+ */
+int bad_to_remove_submodule(const char *path, unsigned flags)
{
ssize_t len;
struct child_process cp = CHILD_PROCESS_INIT;
struct strbuf buf = STRBUF_INIT;
- int ok_to_remove = 1;
+ int ret = 0;
if (!file_exists(path) || is_empty_dir(path))
- return 1;
+ return 0;
if (!submodule_uses_gitfile(path))
- return 0;
+ return 1;
- 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);
+ if (start_command(&cp)) {
+ if (flags & SUBMODULE_REMOVAL_DIE_ON_ERROR)
+ die(_("could not start 'git status in submodule '%s'"),
+ path);
+ ret = -1;
+ goto out;
+ }
len = strbuf_read(&buf, cp.out, 1024);
if (len > 2)
- ok_to_remove = 0;
+ ret = 1;
close(cp.out);
- if (finish_command(&cp))
- die(_("'git status --porcelain -u --ignore-submodules=none' failed in submodule %s"), path);
-
+ if (finish_command(&cp)) {
+ if (flags & SUBMODULE_REMOVAL_DIE_ON_ERROR)
+ die(_("could not run 'git status in submodule '%s'"),
+ path);
+ ret = -1;
+ }
+out:
strbuf_release(&buf);
- return ok_to_remove;
+ return ret;
}
static int find_first_merges(struct object_array *result, const char *path,
diff --git a/submodule.h b/submodule.h
index 61fb610749..21b1569413 100644
--- a/submodule.h
+++ b/submodule.h
@@ -59,7 +59,11 @@ 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_DIE_ON_ERROR (1<<0)
+#define SUBMODULE_REMOVAL_IGNORE_UNTRACKED (1<<1)
+#define SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED (1<<2)
+extern int bad_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.53.gb7b3fba.dirty
^ permalink raw reply related
* [PATCHv5 2/4] submodule: modernize ok_to_remove_submodule to use argv_array
From: Stefan Beller @ 2016-12-20 23:20 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, David.Turner, sandals, j6t, Stefan Beller
In-Reply-To: <20161220232012.15997-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.53.gb7b3fba.dirty
^ permalink raw reply related
* [PATCHv5 1/4] submodule.h: add extern keyword to functions
From: Stefan Beller @ 2016-12-20 23:20 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, David.Turner, sandals, j6t, Stefan Beller
In-Reply-To: <20161220232012.15997-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.53.gb7b3fba.dirty
^ permalink raw reply related
* [PATCHv5 0/4] git-rm absorbs submodule git directory before deletion
From: Stefan Beller @ 2016-12-20 23:20 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, David.Turner, sandals, j6t, Stefan Beller
v5:
* removed the patch for adding {run,start,finish}_or_die
* added one more flag to the function ok_to_remove_submodule
(if die on error is ok)
* renamed ok_to_remove_submodule to bad_to_remove_submodule to signal
the error case better.
v4:
* reworded commit messages of the last 2 patches
* introduced a new patch introducing {run,start,finish}_command_or_die
* found an existing function in dir.h to use to remove a directory
which deals gracefully with the cornercases, that Junio pointed out.
v3:
* removed the patch to enhance ok_to_remove_submodule to absorb the submodule
if needed
* Removed all the error reporting from git-rm that was related to submodule
git directories not absorbed.
* instead just absorb the git repositories or let the absorb function die
with an appropriate error message.
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 (4):
submodule.h: add extern keyword to functions
submodule: modernize ok_to_remove_submodule to use argv_array
submodule: rename and add flags to ok_to_remove_submodule
rm: absorb a submodules git dir before deletion
builtin/rm.c | 83 +++++++++++++++--------------------------------------------
submodule.c | 57 ++++++++++++++++++++++++++--------------
submodule.h | 59 ++++++++++++++++++++++++------------------
t/t3600-rm.sh | 39 +++++++++++-----------------
4 files changed, 108 insertions(+), 130 deletions(-)
--
2.11.0.rc2.53.gb7b3fba.dirty
^ permalink raw reply
* [PATCHv4 0/4] git-rm absorbs submodule git directory before deletion
From: Stefan Beller @ 2016-12-20 23:12 UTC (permalink / raw)
To: gitster
Cc: git, Stefan Beller, Johannes Sixt, Brandon Williams,
brian m. carlson, David Turner
v4:
* removed the patch for adding {run,start,finish}_or_die
* added one more flag to the function ok_to_remove_submodule
(if die on error is ok)
* renamed ok_to_remove_submodule to bad_to_remove_submodule to signal
the error case better.
v3:
* removed the patch to enhance ok_to_remove_submodule to absorb the submodule
if needed
* Removed all the error reporting from git-rm that was related to submodule
git directories not absorbed.
* instead just absorb the git repositories or let the absorb function die
with an appropriate error message.
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
CC: Johannes Sixt <j6t@kdbg.org>
CC: Brandon Williams <bmwill@google.com>
CC: "brian m. carlson" <sandals@crustytoothpaste.net>
CC: David Turner <David.Turner@twosigma.com>
Stefan Beller (4):
submodule.h: add extern keyword to functions
submodule: modernize ok_to_remove_submodule to use argv_array
submodule: rename and add flags to ok_to_remove_submodule
rm: absorb a submodules git dir before deletion
builtin/rm.c | 83 +++++++++++++++--------------------------------------------
submodule.c | 57 ++++++++++++++++++++++++++--------------
submodule.h | 59 ++++++++++++++++++++++++------------------
t/t3600-rm.sh | 39 +++++++++++-----------------
4 files changed, 108 insertions(+), 130 deletions(-)
--
2.11.0.rc2.53.gb7b3fba.dirty
^ permalink raw reply
* Re: [PATCH 00/13] gitk: tweak rendering of remote-tracking references
From: Marc Branchaud @ 2016-12-20 22:53 UTC (permalink / raw)
To: Paul Mackerras; +Cc: Michael Haggerty, git
In-Reply-To: <20161220221719.GB22566@fergus.ozlabs.ibm.com>
On 2016-12-20 05:17 PM, Paul Mackerras wrote:
> On Tue, Dec 20, 2016 at 10:01:15AM -0500, Marc Branchaud wrote:
>> On 2016-12-19 11:44 AM, Michael Haggerty wrote:
>>> This patch series changes a bunch of details about how remote-tracking
>>> references are rendered in the commit list of gitk:
>>
>> Thanks for this! I like the new, compact look very much!
>>
>> That said, I remember when I was a new git user and I leaned heavily on gitk
>> to understand how references worked. It was particularly illuminating to
>> see the remote references distinctly labeled, and the fact that they were
>> "remotes/origin/foo" gave me an Aha! moment where I came to understand that
>> the refs hierarchy is more flexible than just the conventions coded into git
>> itself. I eventually felt free to create my own, private ref hierarchies.
>>
>> I am in no way opposed to this series. I just wanted to point out that
>> there was some utility in those labels. It makes me think that it might be
>> worthwhile for gitk to have a "raw-refs" mode, that shows the full
>> "refs/foo/bar/baz" paths of all the heads, tags, and whatever else. It
>> could be a useful teaching tool for git.
>
> Do you think we should have a checkbox in the preferences dialog to
> select whether to display the long form or the short form?
Mmmm, more knobs!
No, I don't think that's necessary. Such a setting would probably just
confuse people. Plus it's not something anyone is likely to want to
change anyway. Maybe if there were an "Advanced" tab in the settings
dialog, but even then it seems like overkill.
I suspect there are better ways to teach people about the refs hierarchy
than cluttering up gitk. These may even already exist -- I've been a
git user for 8 years now, so I'm sure my perspective of the learning
curve is skewed.
M.
^ permalink raw reply
* Re: [PATCH 00/13] gitk: tweak rendering of remote-tracking references
From: Paul Mackerras @ 2016-12-20 22:17 UTC (permalink / raw)
To: Marc Branchaud; +Cc: Michael Haggerty, git
In-Reply-To: <97d97bc6-54f1-2ef2-fe04-7e7f144d7e51@xiplink.com>
On Tue, Dec 20, 2016 at 10:01:15AM -0500, Marc Branchaud wrote:
> On 2016-12-19 11:44 AM, Michael Haggerty wrote:
> >This patch series changes a bunch of details about how remote-tracking
> >references are rendered in the commit list of gitk:
>
> Thanks for this! I like the new, compact look very much!
>
> That said, I remember when I was a new git user and I leaned heavily on gitk
> to understand how references worked. It was particularly illuminating to
> see the remote references distinctly labeled, and the fact that they were
> "remotes/origin/foo" gave me an Aha! moment where I came to understand that
> the refs hierarchy is more flexible than just the conventions coded into git
> itself. I eventually felt free to create my own, private ref hierarchies.
>
> I am in no way opposed to this series. I just wanted to point out that
> there was some utility in those labels. It makes me think that it might be
> worthwhile for gitk to have a "raw-refs" mode, that shows the full
> "refs/foo/bar/baz" paths of all the heads, tags, and whatever else. It
> could be a useful teaching tool for git.
Do you think we should have a checkbox in the preferences dialog to
select whether to display the long form or the short form?
Paul.
^ permalink raw reply
* Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die
From: Stefan Beller @ 2016-12-20 22:07 UTC (permalink / raw)
To: Johannes Sixt
Cc: Junio C Hamano, git@vger.kernel.org, Brandon Williams,
brian m. carlson, David Turner
In-Reply-To: <48970168-ba4b-e125-6624-e4c37b8a7180@kdbg.org>
On Tue, Dec 20, 2016 at 1:47 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> This does not explain why the *complete and detailed* invocation must be
> reported.
eh. I tried to say that a report that looks like a *complete and
detailed* invocation
should be such, and not be misleading (and e.g. miss one out of 5
arguments printed).
I haven't followed this topic at all, so I may be missing some
> cruical detail. (If you say "it must happen" one more time, then I will
> believe you, because for me that's simpler than to plough through a flock of
> submodule topics. ;-)
It doesn't have to happen; I am trying to come up with a better message.
>
> -- Hannes
>
^ permalink raw reply
* Re: [PATCH] fast-import: properly fanout notes when tree is imported
From: Junio C Hamano @ 2016-12-20 21:50 UTC (permalink / raw)
To: Mike Hommey; +Cc: git, johan
In-Reply-To: <20161220210448.32213-1-mh@glandium.org>
Mike Hommey <mh@glandium.org> writes:
> In typical uses of fast-import, trees are inherited from a parent
> commit. In that case, the tree_entry for the branch looks like:
> ...
> This change makes do_change_note_fanount call load_tree() whenever the
> tree_entry it is given has no tree loaded, making all cases handled
> equally.
Thanks.
^ permalink raw reply
* Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die
From: Johannes Sixt @ 2016-12-20 21:47 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, git@vger.kernel.org, Brandon Williams,
brian m. carlson, David Turner
In-Reply-To: <CAGZ79kYG-veuWNFh6G1g5MiQHBGk2i1qbH_NBtnMS6jFcoWocg@mail.gmail.com>
Am 20.12.2016 um 21:49 schrieb Stefan Beller:
> On Tue, Dec 20, 2016 at 12:12 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> I have to ask, though: Is it so much necessary to report the exact command
>> line in an error message?
>
> Have a look at https://github.com/git/git/blob/master/submodule.c#L1122
>
> die("Could not run 'git status --porcelain -uall \
> --ignore-submodules=none' in submodule %s", path);
>
> There are 2 things:
> 1) The error message is not very informative, as your question hints at.
> I consider changing it to add more meaning. I think the end user
> would also be interested in "Why did we run this command?".
You don't have to. start_command() and finish_command() report any
low-level errors (exec failed, signal received...). If the exit code of
the program is non-zero, finish_command() reports nothing because the
command *itself* will have written some error message ("working
directory dirty", "object database corrupt", "xyz: no such file or
directory"...). At this level, it only makes sense to leave a trace in
which submodule an error occured. So I think it would be sufficient to just
die("could not run 'git status' in submodule %s", path);
or
die("'git status' in submodule %s failed", path);
> 2) We really want to report the correct command line here.
Why? We do not do this anywhere else.
> Currently that is the case, but if you look at the prior patch that extends
> the arguments conditionally (and then also uses the same condition
> to format the error message... that hints at hard to maintain error
> messages.)
This does not explain why the *complete and detailed* invocation must be
reported. I haven't followed this topic at all, so I may be missing some
cruical detail. (If you say "it must happen" one more time, then I will
believe you, because for me that's simpler than to plough through a
flock of submodule topics. ;-)
-- Hannes
^ permalink raw reply
* Re: [PATCH] fast-import: properly fanout notes when tree is imported
From: Mike Hommey @ 2016-12-20 21:06 UTC (permalink / raw)
To: gitster; +Cc: git, johan
In-Reply-To: <20161220210448.32213-1-mh@glandium.org>
Sorry, I forgot the v2 in the subject.
Mike
^ permalink raw reply
* [PATCH] fast-import: properly fanout notes when tree is imported
From: Mike Hommey @ 2016-12-20 21:04 UTC (permalink / raw)
To: gitster; +Cc: git, johan
In-Reply-To: <xmqqbmw6jp59.fsf@gitster.mtv.corp.google.com>
In typical uses of fast-import, trees are inherited from a parent
commit. In that case, the tree_entry for the branch looks like:
.versions[1].sha1 = $some_sha1
.tree = <tree structure loaded from $some_sha1>
However, when trees are imported, rather than inherited, that is not the
case. One can import a tree with a filemodify command, replacing the
root tree object.
e.g.
"M 040000 $some_sha1 \n"
In this case, the tree_entry for the branch looks like:
.versions[1].sha1 = $some_sha1
.tree = NULL
When adding new notes with the notemodify command, do_change_note_fanout
is called to get a notes count, and to do so, it loops over the
tree_entry->tree, but doesn't do anything when the tree is NULL.
In the latter case above, it means do_change_note_fanout thinks the tree
contains no notes, and new notes are added with no fanout.
Interestingly, do_change_note_fanout does check whether subdirectories
have a NULL .tree, in which case it uses load_tree(). Which means the
right behaviour happens when using the filemodify command to import
subdirectories.
This change makes do_change_note_fanount call load_tree() whenever the
tree_entry it is given has no tree loaded, making all cases handled
equally.
Signed-off-by: Mike Hommey <mh@glandium.org>
---
fast-import.c | 8 +++++---
t/t9301-fast-import-notes.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 47 insertions(+), 3 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index cb545d7df5..5e528b1999 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2220,13 +2220,17 @@ static uintmax_t do_change_note_fanout(
char *fullpath, unsigned int fullpath_len,
unsigned char fanout)
{
- struct tree_content *t = root->tree;
+ struct tree_content *t;
struct tree_entry *e, leaf;
unsigned int i, tmp_hex_sha1_len, tmp_fullpath_len;
uintmax_t num_notes = 0;
unsigned char sha1[20];
char realpath[60];
+ if (!root->tree)
+ load_tree(root);
+ t = root->tree;
+
for (i = 0; t && i < t->entry_count; i++) {
e = t->entries[i];
tmp_hex_sha1_len = hex_sha1_len + e->name->str_len;
@@ -2278,8 +2282,6 @@ static uintmax_t do_change_note_fanout(
leaf.tree);
} else if (S_ISDIR(e->versions[1].mode)) {
/* This is a subdir that may contain note entries */
- if (!e->tree)
- load_tree(e);
num_notes += do_change_note_fanout(orig_root, e,
hex_sha1, tmp_hex_sha1_len,
fullpath, tmp_fullpath_len, fanout);
diff --git a/t/t9301-fast-import-notes.sh b/t/t9301-fast-import-notes.sh
index 83acf68bc3..dadc70b7d5 100755
--- a/t/t9301-fast-import-notes.sh
+++ b/t/t9301-fast-import-notes.sh
@@ -483,6 +483,48 @@ test_expect_success 'verify that lots of notes trigger a fanout scheme' '
'
+# Create another notes tree from the one above
+SP=" "
+cat >>input <<INPUT_END
+commit refs/heads/other_commits
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+commit #$(($num_commit + 1))
+COMMIT
+
+from refs/heads/many_commits
+M 644 inline file
+data <<EOF
+file contents in commit #$(($num_commit + 1))
+EOF
+
+commit refs/notes/other_notes
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+committing one more note on a tree imported from a previous notes tree
+COMMIT
+
+M 040000 $(git log --no-walk --format=%T refs/notes/many_notes)$SP
+N inline :$(($num_commit + 1))
+data <<EOF
+note for commit #$(($num_commit + 1))
+EOF
+INPUT_END
+
+test_expect_success 'verify that importing a notes tree respects the fanout scheme' '
+ git fast-import <input &&
+
+ # None of the entries in the top-level notes tree should be a full SHA1
+ git ls-tree --name-only refs/notes/other_notes |
+ while read path
+ do
+ if test $(expr length "$path") -ge 40
+ then
+ return 1
+ fi
+ done
+'
+
cat >>expect_non-note1 << EOF
This is not a note, but rather a regular file residing in a notes tree
EOF
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] fast-import: properly fanout notes when tree is imported
From: Junio C Hamano @ 2016-12-20 20:56 UTC (permalink / raw)
To: Mike Hommey; +Cc: git, johan
In-Reply-To: <20161220204841.awvabgwsxudxfzca@glandium.org>
Mike Hommey <mh@glandium.org> writes:
> On Tue, Dec 20, 2016 at 11:34:04AM -0800, Junio C Hamano wrote:
>> Mike Hommey <mh@glandium.org> writes:
>>
>> > In typical uses of fast-import, trees are inherited from a parent
>> > commit. In that case, the tree_entry for the branch looks like:
>> > ...
>> > +# Create another notes tree from the one above
>> > +cat >>input <<INPUT_END
>> > +...
>> > +M 040000 $(git log --no-walk --format=%T refs/notes/many_notes)
>>
>> There is a trailing SP that cannot be seen by anybody.
>>
>> Don't do this. It makes it very easy to miss what is going on and
>> wastes reviewers' time.
>>
>> Protect it by doing something like:
>>
>> sed -e 's/Z$//' >>input <<INPUT_END
>> ...
>> M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) Z
>
> How about
> EMPTY=
> ...
> M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) $EMPTY
>
> ?
Notice I said "something like" ;-)
I think you are bringing that up to avoid sed, but if you want to go
that route, the long string $EMPTY is distracting, and makes readers
wonder why something that is loud but expands to nothing has to be
there. It hides the true intention, which is that the SP that comes
before it is the most important thing on that line.
I would think a lot more understandable variant would be to do this
instead:
SP=" "
...
M a lot of garbage $(and command)$SP
^ permalink raw reply
* Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die
From: Stefan Beller @ 2016-12-20 20:49 UTC (permalink / raw)
To: Johannes Sixt
Cc: Junio C Hamano, git@vger.kernel.org, Brandon Williams,
brian m. carlson, David Turner
In-Reply-To: <f14ee492-8297-c8ec-f80f-f8f24caf91e1@kdbg.org>
On Tue, Dec 20, 2016 at 12:12 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 20.12.2016 um 20:23 schrieb Stefan Beller:
>>
>> In a reroll I'll drop this patch and instead introduce
>> a function `char *get_child_command_line(struct child_process*);`, which
>> a caller can call before calling finish_command and then use the
>> resulting string
>> to assemble an error message without lego.
>
>
> That sounds a lot better, thank you. Note though, that the function would
> have to be called before start_command(), because when it fails, it would be
> too late.
Yes, the pattern to use it would be
// first assemble the child process struct with conditions
char *cmdline = get_child_command_line(&child)
if (start_command(&child))
// use cmdline here for error reporting.
>
> I have to ask, though: Is it so much necessary to report the exact command
> line in an error message?
Have a look at https://github.com/git/git/blob/master/submodule.c#L1122
die("Could not run 'git status --porcelain -uall \
--ignore-submodules=none' in submodule %s", path);
There are 2 things:
1) The error message is not very informative, as your question hints at.
I consider changing it to add more meaning. I think the end user
would also be interested in "Why did we run this command?".
2) We really want to report the correct command line here.
Currently that is the case, but if you look at the prior patch that extends
the arguments conditionally (and then also uses the same condition
to format the error message... that hints at hard to maintain error
messages.)
So the new proposed function really only addresses 2) here.
> If someone is interested in which command failed,
> it would be "sufficient" to run the command under GIT_TRACE=2.
>
Yes, while at it, I may just move up the error reporting to the builtin
command giving a higher level message.
Thanks,
Stefan
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox