* [PATCH 1/2] mingw: adjust is_console() to work with stdin
From: Johannes Schindelin @ 2016-12-21 17:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pranit Bauva, Johannes Sixt
In-Reply-To: <cover.1482342791.git.johannes.schindelin@gmx.de>
When determining whether a handle corresponds to a *real* Win32 Console
(as opposed to, say, a character device such as /dev/null), we use the
GetConsoleOutputBufferInfo() function as a tell-tale.
However, that does not work for *input* handles associated with a
console. Let's just use the GetConsoleMode() function for input handles,
and since it does not work on output handles fall back to the previous
method for those.
This patch prepares for using is_console() instead of my previous
misguided attempt in cbb3f3c9b1 (mingw: intercept isatty() to handle
/dev/null as Git expects it, 2016-12-11) that broke everything on
Windows.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/winansi.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/compat/winansi.c b/compat/winansi.c
index 5b2f5638ec..97a004b8ab 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -84,6 +84,7 @@ static void warn_if_raster_font(void)
static int is_console(int fd)
{
CONSOLE_SCREEN_BUFFER_INFO sbi;
+ DWORD mode;
HANDLE hcon;
static int initialized = 0;
@@ -98,7 +99,10 @@ static int is_console(int fd)
return 0;
/* check if its a handle to a console output screen buffer */
- if (!GetConsoleScreenBufferInfo(hcon, &sbi))
+ if (!fd) {
+ if (!GetConsoleMode(hcon, &mode))
+ return 0;
+ } else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
return 0;
/* initialize attributes */
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH 0/2] Really fix the isatty() problem on Windows
From: Johannes Schindelin @ 2016-12-21 17:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pranit Bauva, Johannes Sixt
My previous fix may have fixed running the new
t/t6030-bisect-porcelain.sh script that tested the new bisect--helper,
which in turn used isatty() to determine whether it was running
interactively and was fooled by being redirected to /dev/null.
But it not only broke paging when running in a CMD window, due to
testing in the wrong worktree I also missed that it broke paging in Git
for Windows 2.x' Git Bash (i.e. a MinTTY terminal emulator).
Let's use this opportunity to actually clean up the entire isatty() mess
once and for all, as part of the problem was introduced by a clever hack
that messes with internals of the Microsoft C runtime, and which changed
recently, so it was not such a clever hack to begin with.
Happily, one of my colleagues had to address that latter problem
recently when he was tasked to make Git compile with Microsoft Visual C
(the rationale: debugging facilities of Visual Studio are really
outstanding, try them if you get a chance).
And incidentally, replacing the previous hack with the clean, new
solution, which specifies explicitly for the file descriptors 0, 1 and 2
whether we detected an MSYS2 pseudo-tty, whether we detected a real
Win32 Console, and whether we had to swap out a real Win32 Console for a
pipe to allow child processes to inherit it.
Hannes, I am really sorry that I did not test the original attempt
better, I tried to be a better boy this time. Could you maybe also give
it a try?
The current patch series is based on `pu`, as that already has the
winansi_get_osfhandle() fix. For ease of testing, I also have a branch
based on master which you can pull via
git pull https://github.com/dscho/git mingw-isatty-fixup-master
Jeff Hostetler (1):
mingw: replace isatty() hack
Johannes Schindelin (1):
mingw: adjust is_console() to work with stdin
compat/winansi.c | 197 +++++++++++++++++++++++--------------------------------
1 file changed, 83 insertions(+), 114 deletions(-)
base-commit: 1921ea0f1c7358b5200a456fba02aa79fb9e76d3
Based-On: junio/pu at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git junio/pu
Published-As: https://github.com/dscho/git/releases/tag/mingw-isatty-fixup-v1
Fetch-It-Via: git fetch https://github.com/dscho/git mingw-isatty-fixup-v1
--
2.11.0.rc3.windows.1
^ permalink raw reply
* Re: [PATCH] string-list: make compare function compatible with qsort(3)
From: Jeff King @ 2016-12-21 16:12 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano, Johannes Schindelin
In-Reply-To: <c7bac0b7-c555-162f-7880-0355831cee48@web.de>
On Wed, Dec 21, 2016 at 10:36:41AM +0100, René Scharfe wrote:
> One shortcoming is that the comparison function is restricted to working
> with the string members of items; util is inaccessible to it. Another
> one is that the value of cmp is passed in a global variable to
> cmp_items(), making string_list_sort() non-reentrant.
I think this approach is OK for string_list, but it doesn't help the
general case that wants qsort_s() to actually access global data. I
don't know how common that is in our codebase, though.
So I'm fine with it, but I think we might eventually need to revisit the
qsort_s() thing anyway.
> Remove the intermediate layer, i.e. cmp_items(), make the comparison
> functions compatible with qsort(3) and pass them pointers to full items.
> This allows comparisons to also take the util member into account, and
> avoids the need to pass the real comparison function to an intermediate
> function, removing the need for a global function.
I'm not sure if access to the util field is really of any value, after
looking at it in:
http://public-inbox.org/git/20161125171546.fa3zpapbjngjcl26@sigill.intra.peff.net/
Though note that if we do take this patch, there are probably one or two
spots that could switch from QSORT() to string_list_sort().
-Peff
^ permalink raw reply
* Re: Allow "git shortlog" to group by committer information
From: Jeff King @ 2016-12-21 16:04 UTC (permalink / raw)
To: Jacob Keller
Cc: Junio C Hamano, Johannes Sixt, Linus Torvalds, Git Mailing List
In-Reply-To: <CA+P7+xrMgzFcuqwBg6z2_ZPgAVKwLX2eyK6D4C0v-c3zAMFqUg@mail.gmail.com>
On Tue, Dec 20, 2016 at 11:55:45PM -0800, Jacob Keller wrote:
> > 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?
Yes, it definitely means the skip-able tests would have to be careful.
But it's putting the onus on them, rather than on all the other tests.
If the rule is "the on-disk state must be the same whether or not the
test runs", then I suspect many tests could get by with a
test_when_finished, like:
test_expect_success FOO 'test --foo knob' '
git commit -m "new commit for test" &&
test_when_finished "git reset --hard HEAD^" &&
git log --foo >actual &&
test_cmp expect actual
'
It gets harder if you have multiple such tests that rely on intermediate
state. Probably you'd have:
test_expect_success FOO 'clean up FOO state' '
git reset --hard HEAD^
'
at the end of the sequence.
I dunno. It is unclear to me whether such a rule is worth it.
Preemptively fighting these state-based bugs before they occur is a nice
thought, but I think it may end up being more work to write the tests
that way than it is to simply find and fix the bugs when they occur. Of
course it also changes where the work falls (the test writers versus the
bug hunters, and given that !MINGW is our most common prereq, I think
Windows devs are over-represented in the latter case).
-Peff
^ permalink raw reply
* Re: [PATCH] mailinfo.c: move side-effects outside of assert
From: Jeff King @ 2016-12-21 15:55 UTC (permalink / raw)
To: Kyle J. McKay
Cc: Johannes Schindelin, Jonathan Tan, Junio C Hamano,
Git mailing list
In-Reply-To: <222ACFD4-ED9A-4B94-8BDD-3C70648A684B@gmail.com>
On Tue, Dec 20, 2016 at 09:54:15PM -0800, Kyle J. McKay wrote:
> > 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? ;)
Kind of. If it's a configuration that nobody[1] in the Git development
community intended to support or test, then isn't the person triggering
it the one making assumptions?
At any rate, I agree that setting NDEBUG should not create a broken
program, and some solution like your patch is a good idea here. I was
mainly speaking to the "do not bother" comment. It is not that I do not
bother to build with NDEBUG, it is that I think it is actively a bad
idea.
[1] Maybe I am alone in my surprise, and everybody working on Git is
using assert() with the intention that it can be disabled. But if
that were the case, I'd expect more push-back against "die(BUG)"
which does not have this feature. I don't recall a single discussion
to that effect, and searching for NDEBUG in the list archives turns
up hardly any mentions.
> 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.
I think here you are getting into superstition. Is there any single
assert() in Git that will actually have an impact on performance?
I'd be more impressed if you could show some operation that is faster
when built with NDEBUG than without. Running all of t/perf does not seem
to show any difference, and looking at the asserts themselves, they're
almost all single-instruction compares in code that isn't performance
critical anyway.
> > 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.
I hope my history of patches shows that I do care about deploying Git in
a performance critical environment. But I only care about performance
tradeoffs that have a _measurable_ gain.
> 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.
What would be the advantage of that over `if(...) die("BUG: ...")`? It
does not require you to write a reason in the die(), but I am not sure
that is a good thing.
> > 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.
AFAICT, yes. I'd leave it to people who actually build with nedmalloc to
decide whether it is worth caring about, and whether the asserts there
have a noticeable performance impact.
-Peff
^ permalink raw reply
* Re: Races on ref .lock files?
From: Andreas Krey @ 2016-12-21 10:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqpokru6yg.fsf@gitster.mtv.corp.google.com>
On Fri, 16 Dec 2016 09:20:07 +0000, Junio C Hamano wrote:
...
> > error: cannot lock ref 'stash-refs/pull-requests/18978/to': Unable to create '/opt/apps/..../repositories/68/stash-refs/pull-requests/18978/to.lock': File exists.
...
> I think (and I think you also think) these messages come from the
> Bitbucket side, not your "git push" (or "git fetch").
I *know* that this is the case - we don't have refs like that
on the local side. (Our users are more scared about them.)
...
> when there are locked refs. I'd naively think that unless you are
> pushing to that ref you showed an error message for, the receiving
> end shouldn't care if the ref is being written by somebody else, but
> who knows ;-) They may have their own reasons wanting to lock that
> ref that we think would be irrelevant for the operation, causing
> errors.
Possible. I'm going Byrans way for now, disabling the gc there.
But:
In a different instance, we have a simple bare git repo that we
use for backup purposes. Which means there are lots of pushes
going there (all to disjunct refs), and I now cared to look
into those logfiles:
----snip
Wed Dec 21 05:08:14 CET 2016
fatal: Unable to create '/data/git-backup/backup.git/packed-refs.lock': File exists.
If no other git process is currently running, this probably means a
git process crashed in this repository earlier. Make sure no other git
process is running and remove the file manually to continue.
error: failed to run pack-refs
To git-backup-user@socrepo.advantest.com:backup.git
+ 8aac9ae...2df6d56 refs/zz/current -> refs/backup/socvm217/ZworkspacesZsocvm217ZjohanabtZws-release_tools.Ycurr (forced update)
----snip
I interpret this as "I updated the refs files, but packing them
didn't work because someone else was also packing right now."
Is that happening as designed, or do I need to be afraid
that some refs didn't make the push?
To ask differently, is git relying on people reading such
messages and following up on them? And thus isn't that
easy to use in automated processes? (Additional problem:
The user in question, besides being an automat, doesn't
have the capability to work in the target repository.)
Andreas
--
"Totally trivial. Famous last words."
From: Linus Torvalds <torvalds@*.org>
Date: Fri, 22 Jan 2010 07:29:21 -0800
^ permalink raw reply
* [PATCH] string-list: make compare function compatible with qsort(3)
From: René Scharfe @ 2016-12-21 9:36 UTC (permalink / raw)
To: Git List; +Cc: Jeff King, Junio C Hamano, Johannes Schindelin
The cmp member of struct string_list points to a comparison function
that is used for sorting and searching of list items. It takes two
string pointers -- like strcmp(3), which is in fact the default;
cmp_items() provides a qsort(3) compatible interface by passing the
string members of two struct string_list_item pointers to cmp.
One shortcoming is that the comparison function is restricted to working
with the string members of items; util is inaccessible to it. Another
one is that the value of cmp is passed in a global variable to
cmp_items(), making string_list_sort() non-reentrant.
Remove the intermediate layer, i.e. cmp_items(), make the comparison
functions compatible with qsort(3) and pass them pointers to full items.
This allows comparisons to also take the util member into account, and
avoids the need to pass the real comparison function to an intermediate
function, removing the need for a global function.
A downside is that comparison functions take void pointers now and each
of them needs to cast its arguments to struct string_list_item pointers,
weakening type safety and adding some repetitive code. Programmers are
used to that, however, as that's par for the course with qsort(3).
Also two unsightly casts are added that remove the const qualifiers of
strings while building the structs to pass to the comparison function as
search keys. It should be safe, though, as we only ever use them for
reading.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
Alternative approach to the qsort_s()-based series "string-list: make
string_list_sort() reentrant".
Documentation/technical/api-string-list.txt | 6 +++--
builtin/mailsplit.c | 5 +++-
mailmap.c | 5 ++--
merge-recursive.c | 4 ++-
string-list.c | 39 +++++++++++++++--------------
string-list.h | 4 +--
tmp-objdir.c | 4 ++-
7 files changed, 39 insertions(+), 28 deletions(-)
diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt
index c08402b12..39eac59c7 100644
--- a/Documentation/technical/api-string-list.txt
+++ b/Documentation/technical/api-string-list.txt
@@ -205,5 +205,7 @@ Represents the list itself.
You should not tamper with it.
. Setting the `strdup_strings` member to 1 will strdup() the strings
before adding them, see above.
-. The `compare_strings_fn` member is used to specify a custom compare
- function, otherwise `strcmp()` is used as the default function.
+. The `cmp` member is used to specify a custom compare function. It has
+ the same signature as the one for qsort(1) and is passed two pointers
+ to `struct string_list_item`. If it's NULL then the `string` members
+ are compared with `strcmp(1)`; this is the default.
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 30681681c..4e72e3128 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -147,8 +147,11 @@ static int populate_maildir_list(struct string_list *list, const char *path)
return ret;
}
-static int maildir_filename_cmp(const char *a, const char *b)
+static int maildir_filename_cmp(const void *one, const void *two)
{
+ const struct string_list_item *item_one = one, *item_two = two;
+ const char *a = item_one->string, *b = item_two->string;
+
while (*a && *b) {
if (isdigit(*a) && isdigit(*b)) {
long int na, nb;
diff --git a/mailmap.c b/mailmap.c
index c1a79c100..5290b5153 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -61,9 +61,10 @@ static void free_mailmap_entry(void *p, const char *s)
* namemap.cmp until we know no systems that matter have such an
* "unusual" string.h.
*/
-static int namemap_cmp(const char *a, const char *b)
+static int namemap_cmp(const void *one, const void *two)
{
- return strcasecmp(a, b);
+ const struct string_list_item *item_one = one, *item_two = two;
+ return strcasecmp(item_one->string, item_two->string);
}
static void add_mapping(struct string_list *map,
diff --git a/merge-recursive.c b/merge-recursive.c
index d32720944..4683ba43f 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -390,8 +390,10 @@ static struct string_list *get_unmerged(void)
return unmerged;
}
-static int string_list_df_name_compare(const char *one, const char *two)
+static int string_list_df_name_compare(const void *a, const void *b)
{
+ const struct string_list_item *item_a = a, *item_b = b;
+ const char *one = item_a->string, *two = item_b->string;
int onelen = strlen(one);
int twolen = strlen(two);
/*
diff --git a/string-list.c b/string-list.c
index 8c83cac18..c583a04ee 100644
--- a/string-list.c
+++ b/string-list.c
@@ -7,17 +7,26 @@ void string_list_init(struct string_list *list, int strdup_strings)
list->strdup_strings = strdup_strings;
}
+static int string_list_item_strcmp(const void *one, const void *two)
+{
+ const struct string_list_item *item_one = one, *item_two = two;
+ return strcmp(item_one->string, item_two->string);
+}
+
/* if there is no exact match, point to the index where the entry could be
* inserted */
static int get_entry_index(const struct string_list *list, const char *string,
int *exact_match)
{
int left = -1, right = list->nr;
- compare_strings_fn cmp = list->cmp ? list->cmp : strcmp;
+ compare_fn_t cmp = list->cmp ? list->cmp : string_list_item_strcmp;
+ struct string_list_item key = { NULL };
+
+ key.string = (char *)string;
while (left + 1 < right) {
int middle = (left + right) / 2;
- int compare = cmp(string, list->items[middle].string);
+ int compare = cmp(&key, &list->items[middle]);
if (compare < 0)
right = middle;
else if (compare > 0)
@@ -94,11 +103,11 @@ struct string_list_item *string_list_lookup(struct string_list *list, const char
void string_list_remove_duplicates(struct string_list *list, int free_util)
{
+ compare_fn_t cmp = list->cmp ? list->cmp : string_list_item_strcmp;
if (list->nr > 1) {
int src, dst;
- compare_strings_fn cmp = list->cmp ? list->cmp : strcmp;
for (src = dst = 1; src < list->nr; src++) {
- if (!cmp(list->items[dst - 1].string, list->items[src].string)) {
+ if (!cmp(&list->items[dst - 1], &list->items[src])) {
if (list->strdup_strings)
free(list->items[src].string);
if (free_util)
@@ -211,31 +220,23 @@ struct string_list_item *string_list_append(struct string_list *list,
list->strdup_strings ? xstrdup(string) : (char *)string);
}
-/* Yuck */
-static compare_strings_fn compare_for_qsort;
-
-/* Only call this from inside string_list_sort! */
-static int cmp_items(const void *a, const void *b)
-{
- const struct string_list_item *one = a;
- const struct string_list_item *two = b;
- return compare_for_qsort(one->string, two->string);
-}
-
void string_list_sort(struct string_list *list)
{
- compare_for_qsort = list->cmp ? list->cmp : strcmp;
- QSORT(list->items, list->nr, cmp_items);
+ compare_fn_t cmp = list->cmp ? list->cmp : string_list_item_strcmp;
+ QSORT(list->items, list->nr, cmp);
}
struct string_list_item *unsorted_string_list_lookup(struct string_list *list,
const char *string)
{
struct string_list_item *item;
- compare_strings_fn cmp = list->cmp ? list->cmp : strcmp;
+ compare_fn_t cmp = list->cmp ? list->cmp : string_list_item_strcmp;
+ struct string_list_item key = { NULL };
+
+ key.string = (char *)string;
for_each_string_list_item(item, list)
- if (!cmp(string, item->string))
+ if (!cmp(&key, item))
return item;
return NULL;
}
diff --git a/string-list.h b/string-list.h
index d3809a141..073025ddc 100644
--- a/string-list.h
+++ b/string-list.h
@@ -6,13 +6,13 @@ struct string_list_item {
void *util;
};
-typedef int (*compare_strings_fn)(const char *, const char *);
+typedef int (*compare_fn_t)(const void *, const void *);
struct string_list {
struct string_list_item *items;
unsigned int nr, alloc;
unsigned int strdup_strings:1;
- compare_strings_fn cmp; /* NULL uses strcmp() */
+ compare_fn_t cmp; /* NULL uses strcmp() on ->string */
};
#define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0, NULL }
diff --git a/tmp-objdir.c b/tmp-objdir.c
index 64435f23a..b6209b199 100644
--- a/tmp-objdir.c
+++ b/tmp-objdir.c
@@ -173,8 +173,10 @@ static int pack_copy_priority(const char *name)
return 4;
}
-static int pack_copy_cmp(const char *a, const char *b)
+static int pack_copy_cmp(const void *one, const void *two)
{
+ const struct string_list_item *item_one = one, *item_two = two;
+ const char *a = item_one->string, *b = item_two->string;
return pack_copy_priority(a) - pack_copy_priority(b);
}
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 1/3] compat: add qsort_s()
From: René Scharfe @ 2016-12-21 9:36 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Git List, Johannes Schindelin
In-Reply-To: <20161212195703.nmljhwwmrn56gbyd@sigill.intra.peff.net>
Am 12.12.2016 um 20:57 schrieb Jeff King:
> On Mon, Dec 12, 2016 at 08:51:14PM +0100, René Scharfe wrote:
>
>> It's kinda cool to have a bespoke compatibility layer for major platforms,
>> but the more I think about it the less I can answer why we would want that.
>> Safety, reliability and performance can't be good reasons -- if our fallback
>> function lacks in these regards then we have to improve it in any case.
>
> There may be cases that we don't want to support because of portability
> issues. E.g., if your libc has an assembly-optimized qsort() we wouldn't
> want to replicate that.
Offloading to GPUs might be a better example; I don't know of a libc
that does any of that, though (yet).
> I dunno. I am not that opposed to just saying "forget libc qsort, we
> always use our internal one which is consistent, performant, and safe".
> But when I suggested something similar for our regex library, I seem to
> recall there were complaints.
Well, I'm not sure how comparable they are, but perhaps we can avoid
compat code altogether in this case. Patch coming in a new thread.
René
^ permalink raw reply
* Re: Races on ref .lock files?
From: Andreas Krey @ 2016-12-21 8:33 UTC (permalink / raw)
To: Bryan Turner; +Cc: Git Users
In-Reply-To: <CAGyf7-EvbV4XWCsfLpCUDo5V4_wM3SSJcHxVh9Rp78JUC6S-yw@mail.gmail.com>
On Fri, 16 Dec 2016 15:34:22 +0000, Bryan Turner wrote:
...
> Bitbucket Server developer here.
Social media rock. :-)
> If you'd like to investigate more in depth, I'd encourage you to
> create a ticket on support.atlassian.com so we can work with you.
That is going to be postponed until we update our bitbucket instance
to the current state.
> Otherwise, if you just want to prevent seeing these messages, you can
> either fork the relevant repository in Bitbucket Server (which
> disables auto GC), or run "git config gc.auto 0" in
Doing that for now. Will come back either if it doesn't help,
or after the upgrade.
> within Bitbucket Server instead, so a future upgrade should
> automatically prevent these messages from appearing on clients.
I still wonder if git itself should prevent these, or is there
a (git level) recommendation not to enable auto-gc in repos where
people regularly push to?
- Andreas
--
"Totally trivial. Famous last words."
From: Linus Torvalds <torvalds@*.org>
Date: Fri, 22 Jan 2010 07:29:21 -0800
^ permalink raw reply
* 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
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