Re: de-alphabetizing the documentation
Hi Jonathan, If it's really just a matter of needing someone with a newcomer's perspective, then I'd be happy to look over the ordering of the git subcommands. You can run the command I provided to glean the frequency of each subcommand from your shell history, I'll look over the output and see if the ordering makes sense to me, and then you or someone else can rearrange the manual page to list the subcommands in this order. Is that a suitable plan? Thanks, Frederick On Fri, Jul 06, 2018 at 04:47:15PM -0700, Jonathan Nieder wrote: > Hi, > > Frederick Eaton wrote: > > > Unfortunately my contribution will have to be limited for the > > moment to making this suggestion, as I am extraordinarily busy. I hope > > it will not be too burdensome to add this item to your TODO list and > > keep it there until a willing volunteer comes along. > > No problem. If you have time to contribute later, we can wait. :) > > > For what it's worth, I made extensive changes to the Arch Wiki Git > > article back in 2015, following an initial attempt of mine to > > understand various tutorials. It was the most prominent wiki-based Git > > documentation I could find at the time. The article has of course seen > > numerous improvements since then. > > For reference: https://wiki.archlinux.org/index.php/git > > > I don't think that it's really important to find a "best" ordering for > > commands or glossary terms; it's more a matter of finding someone who > > is willing to take responsibility for choosing a reasonable ordering. > > Presumably the head maintainer of this project could delegate the task > > to a qualified volunteer, not a newbie like myself but not necessarily > > someone with expert knowledge either. > > I'd have to say, when I compare the troubles a new user and a > long-timer would run into, I conclude that the long-timers would be > more likely to produce worse documentation. It is very difficult to > remember how new users see things. The ideal skill set in fact has > nothing to do with level of Git expertise: to produce a good result, a > good technical writer would ask the right questions to gather > information from the experts and then test their documentation on > newcomers until it works well. > > Based on the work you've described already having done, it sounds like > you'd be an ideal person to get this going, if you find yourself with > time for it. > > > It's too bad that a policy of > > not listing things alphabetically wasn't adopted from the beginning of > > this project, but I guess that's life. > > From this thread I've been convinced that for this kind of reference > document, alphabetical organization within each section is a good > organization, provided each section is small enough (as in "git help" > output). > > I'm also a fan of non reference documentation that can use a narrative > ordering instead (like "git help core-tutorial", except with more > modern commands). > > > Thanks Eric for the pointer to "git help". This does indeed provide a > > finer and better grouping than the man-page (but it also looks like > > another candidate for de-alphabetization...!). > > Indeed, copying that organization over from "git help" to the git(1) > manpage may be a good step for any interested people overhearing this > conversation. > > As a first step, how about making git(1) recommend "git help", like > this? It already recommends giteveryday(7) but the more interactive > first command might be useful for some people. > > Thoughts? Improvements? > > -- >8 -- > Subject: git doc: recommend "git help" as a starting point > > The list of subcommands described in git(1) can be overwhelming. > Encourage newcomers to run "git help" to get a shorter list of > commands as a starting point. > > Based on a suggestion by Frederick Eaton. > > Signed-off-by: Jonathan Nieder > --- > Documentation/git.txt | 9 ++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/Documentation/git.txt b/Documentation/git.txt > index dba7f0c18e..0149ce9af0 100644 > --- a/Documentation/git.txt > +++ b/Documentation/git.txt > @@ -23,9 +23,12 @@ unusually rich command set that provides both high-level > operations > and full access to internals. > > See linkgit:gittutorial[7] to get started, then see > -linkgit:giteveryday[7] for a useful minimum set of > -commands. The link:user-manual.html[Git User's Manual] has a more > -in-depth introduction. > +linkgit:giteveryday[7] and run > + > +git help > + > +for a useful minimum set of commands. The link:user-manual.html[Git > +User's Manual] has a more in-depth introduction. > > After you mastered the basic concepts, you can come back to this > page to learn what commands Git offers. You can learn more about > -- > 2.18.0.203.gfac676dfb9 >
Re: [PATCH] builtin/config: work around an unsized array forward declaration
On (06/07/18 12:24), Junio C Hamano wrote: > > Jeff King writes: > > > On Thu, Jul 05, 2018 at 09:50:53PM +0200, Beat Bolli wrote: > > > >> > Your patch is obviously correct, but I think here there might be an even > >> > simpler solution: just bump option_parse_type() below the declaration, > >> > since it's the only one that needs it. That hunk is bigger, but the > >> > overall diff is simpler, and we don't need to carry that extra wrapper > >> > function. > >> > >> That was dscho's first try in the GitHub issue. It doesn't compile > >> because the OPT_CALLBACK* macros in the builtin_config_options > >> declaration inserts a pointer to option_parse_type into the array items. > >> We need at least one forward declaration, and my patch seemed the least > >> intrusive. > > > > Ah, right, so it actually is mutually recursive. Forward-declaring > > option_parse_type() would fix it, along with the reordering. I'm > > ambivalent between the available options, then; we might as well go with > > what you posted, then, since it's already done. :) > > Among three, forward declaration of the function with reordering > that nobody has written except for in the brain smells the best, and > turning an array to a pointer that points at a separate storage looked > the worst. I also am OK with what's already posted, too. I posted the forward declaration of the function on the Git for Windows issue: https://github.com/git-for-windows/git/issues/1735#issuecomment-402825623 I would consider it the minimal fix. The already posted solution is also OK for me. It's also possible to use "extern" instead of "static" for the array. It would, however, not be my preferred solution. -Kim
Re: [PATCH] gc --auto: release pack files before auto packing
On (07/07/18 11:40), SZEDER Gábor wrote:
>
> On Fri, Jul 6, 2018 at 6:39 PM Junio C Hamano wrote:
> >
> > Duy Nguyen writes:
> >
> > > On Sat, Jun 30, 2018 at 03:38:21PM +0200, Kim Gybels wrote:
> > >> Teach gc --auto to release pack files before auto packing the repository
> > >> to prevent failures when removing them.
> > >>
> > >> Also teach the test 'fetching with auto-gc does not lock up' to complain
> > >> when it is no longer triggering an auto packing of the repository.
> > >>
> > >> Fixes https://github.com/git-for-windows/git/issues/500
> > >>
> > >> Signed-off-by: Kim Gybels
> > >> ---
> > >>
> > >> Patch based on master, since problem doesn't reproduce on maint,
> > >> however, the improvement to the test might be valuable on maint.
> > >>
> > >> builtin/gc.c | 1 +
> > >> t/t5510-fetch.sh | 2 ++
> > >> 2 files changed, 3 insertions(+)
> > >>
> > >> diff --git a/builtin/gc.c b/builtin/gc.c
> > >> index ccfb1ceaeb..63b62ab57c 100644
> > >> --- a/builtin/gc.c
> > >> +++ b/builtin/gc.c
> > >> @@ -612,6 +612,7 @@ int cmd_gc(int argc, const char **argv, const char
> > >> *prefix)
> > >> return -1;
> > >>
> > >> if (!repository_format_precious_objects) {
> > >> +close_all_packs(the_repository->objects);
> > >
> > > We have repo_clear() which does this and potentially closing file
> > > descriptors on other things as well. I suggest we use it, and before
> > > any external command is run. Something like
> > >
> > > diff --git a/builtin/gc.c b/builtin/gc.c
> > > index ccfb1ceaeb..a852c71da6 100644
> > > --- a/builtin/gc.c
> > > +++ b/builtin/gc.c
> > > @@ -473,6 +473,13 @@ static int report_last_gc_error(void)
> > >
> > > static int gc_before_repack(void)
> > > {
> > > + /*
> > > + * Shut down everything, we should have all the info we need
> > > + * at this point. Leaving some file descriptors open may
> > > + * prevent them from being removed on Windows.
> > > + */
> > > + repo_clear(the_repository);
> > > +
> > > if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
> > > return error(FAILED_RUN, pack_refs_cmd.argv[0]);
> >
> > With
> >
> > https://public-inbox.org/git/20180509170409.13666-1-pclo...@gmail.com/
> >
> > the call to repo_clear() on the_repository itself may be safe [*1*],
> > but if command line parsing code etc. has pointers to in-core object
> > etc. obtained before this function was called, the codeflow after
> > this function returns will be quite disturbed. Has anybody in this
> > review thread audited the codeflow _after_ this function is run to
> > make sure that invalidating in-core repository here does not cause
> > us problems?
>
> It does create us problems, unfortunately.
>
> Among other things, a call to repo_clear(the_repository) does this:
>
> raw_object_store_clear(repo->objects);
> FREE_AND_NULL(repo->objects);
>
> Later on cmd_gc() calls reprepare_packed_git(the_repository), which
> then attempts to:
>
> r->objects->approximate_object_count_valid = 0;
> r->objects->packed_git_initialized = 0;
>
> but with r->objects being NULL at this point that won't work, and in
> turn causes failures in several test scripts.
>
>
> FWIW, the original patch appears to be working fine, at least the test
> suite passes.
Should I post a v3 that goes back to the original fix, but uses
test_i18ngrep instead of grep?
In addition to not breaking any tests, close_all_packs is already used
in a similar way in am and fetch just before running "gc --auto".
-Kim
Re: What's (not) cooking
On (07/07/18 08:34), Elijah Newren wrote:
> Hi Dscho,
>
> On Sat, Jul 7, 2018 at 5:11 AM, Johannes Schindelin
> wrote:
> > Hi Elijah,
> >
> > On Fri, 6 Jul 2018, Elijah Newren wrote:
> >
> >> On Fri, Jul 6, 2018 at 3:57 PM, Junio C Hamano wrote:
> >> > I'll be pushing out the integration branches with some updates, but
> >> > there is no change in 'next' and below. The following topics I gave
> >> > a quick look and gave them topic branches, but I had trouble merging
> >> > them in 'pu' and making them work correctly or pass the tests, so
> >> > they are not part of 'pu' in today's pushout.
> >> >
> >> > pk/rebase-in-c
> >> > en/dirty-merge-fixes
> >> > en/t6036-merge-recursive-tests
> >> > en/t6042-insane-merge-rename-testcases
> >> > ds/multi-pack-index
> >>
> >> It looks to me like the main problem is that pu itself has lots of
> >> test failures. It seems to bisect down to
> >> kg/gc-auto-windows-workaround. If I revert commit ac9d3fdbebbd ("gc
> >> --auto: clear repository before auto packing", 2018-07-04), then pu
> >> passes tests again for me.
> >
> > Is this the segmentation fault about which I just sent a mail?
>
> Yes, this is is a gc segfault issue. It looks like every test calling
> git gc will fail because of it, I even saw a "nothing to pack" message
> or something like that followed by a segfault.
Sorry, I forgot to run the tests for v2 of my patch. I'll be more
careful in the future.
-Kim
Re: [PATCH v3 01/20] linear-assignment: a function to solve least-cost assignment problems
Hi Junio, On Sat, 7 Jul 2018, Johannes Schindelin wrote: > On Sat, 7 Jul 2018, Junio C Hamano wrote: > > > Johannes Schindelin writes: > > > > >> Does the "gitgitgadget" thing lie on the Date: e-mail header? > > > > > > No, GitGitGadget takes the literal output from `git format-patch`, as far > > > as I can tell. So if at all, it is `format-patch` that is lying. > > > > format-patch faithfully records the fact about the commit that is > > made into the patch. How pieces of information should (or should > > not) be used depends on the purpose of the application that uses > > its output. > > I guess this is one of the fallouts for abusing the `format-patch|am` > dance for `rebase--am`. Speaking of GitGitGadget: I just encoutered a problem with your `refs/notes/amlog` and I hope you can help me with that. Concretely, I want GitGitGadget to be able to identify the commit that corresponds to a given mail that contained a patch (if it ever made it into `pu`), to automate all kinds of tedious things that I currently have to perform manually. And here I hit a block: I am looking for the commit corresponding to aca087479b35cbcbd7c84c7ca3bcf556133d0548.1530274571.git.gitgitgad...@gmail.com When I ask `git notes --ref=refs/notes/gitster-amlog show 4cec3986f017d84c8d6a2c4233d2eba4a3ffa60d` (the SHA-1 is the one corresponding to `Message-Id: <...>` for that mail), it insists on outputting 5902152ab02291af4454f24a8ccaf2adddefc306 However, I cannot find that commit anywhere. When I look for the commit in the same manual, tedious way that I want to automate, I find that it *is* in `pu`, but as 5cf8e064747be2026bb23be37f84f2f0b2a31781 Even curiouser: when I now ask for the commit notes for both of those SHA-1s, I get back the correct, same Message-Id *for both of them*, which makes me think that it was recorded correctly, but then overwritten due to some process I don't understand. Would you be able to shed light into this? Thank you, Dscho
[RFC PATCH v2] Add 'human' date format
From: Linus Torvalds
This adds --date=human, which skips the timezone if it matches the
current time-zone, and doesn't print the whole date if that matches (ie
skip printing year for dates that are "this year", but also skip the
whole date itself if it's in the last few days and we can just say what
weekday it was).
For really recent dates (same day), use the relative date stamp, while
for old dates (year doesn't match), don't bother with time and timezone.
Also add 'auto' date mode, which defaults to human if we're using the
pager. So you can do
git config --add log.date auto
and your "git log" commands will show the human-legible format unless
you're scripting things.
Note that this time format still shows the timezone for recent enough
events (but not so recent that they show up as relative dates). You can
combine it with the "-local" suffix to never show timezones for an even
more simplified view.
Signed-off-by: Linus Torvalds
---
Slightly updated version after playing with this more.
This tries to make the length somewhat more consistent (and shorter),
which came about when looking at this in "git blame" output.
Once you're talking "last year" patches, you don't tend to care about time
of day or timezone. So the longest date is basically "Thu Oct 19 16:00",
because if you show the year (four characters), you don't show the time
(five characters). And the timezone (five characters) is only shown if not
showing the date (5-6 characters).
Also, because the relative time is now handled entirely inside the
show_date_normal() function, I could undo some of the changes to
show_date() that were updating date->mode and date->local. So the patch
has actually shrunk a bit, I think.
builtin/blame.c | 4 ++
cache.h | 1 +
date.c | 130
3 files changed, 115 insertions(+), 20 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 5a0388aae..7b6235321 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -917,6 +917,10 @@ int cmd_blame(int argc, const char **argv, const char
*prefix)
*/
blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) +
1; /* add the null */
break;
+ case DATE_HUMAN:
+ /* If the year is shown, no time is shown */
+ blame_date_width = sizeof("Thu Oct 19 16:00");
+ break;
case DATE_NORMAL:
blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");
break;
diff --git a/cache.h b/cache.h
index d49092d94..8a6810ee6 100644
--- a/cache.h
+++ b/cache.h
@@ -1428,6 +1428,7 @@ extern struct object *peel_to_type(const char *name, int
namelen,
struct date_mode {
enum date_mode_type {
DATE_NORMAL = 0,
+ DATE_HUMAN,
DATE_RELATIVE,
DATE_SHORT,
DATE_ISO8601,
diff --git a/date.c b/date.c
index 49f943e25..4486c028a 100644
--- a/date.c
+++ b/date.c
@@ -77,22 +77,16 @@ static struct tm *time_to_tm_local(timestamp_t time)
}
/*
- * What value of "tz" was in effect back then at "time" in the
- * local timezone?
+ * Fill in the localtime 'struct tm' for the supplied time,
+ * and return the local tz.
*/
-static int local_tzoffset(timestamp_t time)
+static int local_time_tzoffset(time_t t, struct tm *tm)
{
- time_t t, t_local;
- struct tm tm;
+ time_t t_local;
int offset, eastwest;
- if (date_overflows(time))
- die("Timestamp too large for this system: %"PRItime, time);
-
- t = (time_t)time;
- localtime_r(, );
- t_local = tm_to_time_t();
-
+ localtime_r(, tm);
+ t_local = tm_to_time_t(tm);
if (t_local == -1)
return 0; /* error; just use + */
if (t_local < t) {
@@ -107,6 +101,20 @@ static int local_tzoffset(timestamp_t time)
return offset * eastwest;
}
+/*
+ * What value of "tz" was in effect back then at "time" in the
+ * local timezone?
+ */
+static int local_tzoffset(timestamp_t time)
+{
+ struct tm tm;
+
+ if (date_overflows(time))
+ die("Timestamp too large for this system: %"PRItime, time);
+
+ return local_time_tzoffset((time_t)time, );
+}
+
void show_date_relative(timestamp_t time, int tz,
const struct timeval *now,
struct strbuf *timebuf)
@@ -191,9 +199,80 @@ struct date_mode *date_mode_from_type(enum date_mode_type
type)
return
}
+static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm
*tm, int tz, struct tm *human_tm, int human_tz, int local)
+{
+ struct {
+ unsigned intyear:1,
+ date:1,
+ wday:1,
+ time:1,
+ seconds:1,
+ tz:1;
+
[PATCH 2/3] t7405: add a directory/submodule conflict
For a directory/submodule conflict, we want contents from both the directory and the submodule to be present for the user to use to resolve the conflict, but we do not want paths under the directory being written into the submodule and we do not want the merge being confused by paths under the submodule being in the way. Add testcases for these situations. Signed-off-by: Elijah Newren --- t/t7405-submodule-merge.sh | 91 ++ 1 file changed, 91 insertions(+) diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh index 95fd05d83..6cb51c966 100755 --- a/t/t7405-submodule-merge.sh +++ b/t/t7405-submodule-merge.sh @@ -335,4 +335,95 @@ test_expect_failure 'file/submodule conflict' ' ) ' +# Directory/submodule conflict +# Commit O: +# Commit A: path (submodule), with sole tracked file named 'world' +# Commit B1: path/file +# Commit B2: path/world +# +# Expected from merge of A & B1: +# Contents under path/ from commit B1 are renamed elsewhere; we do not +# want to write files from one of our tracked directories into a submodule +# +# Expected from merge of A & B2: +# Similar to last merge, but with a slight twist: we don't want paths +# under the submodule to be treated as untracked or in the way. + +test_expect_success 'setup directory/submodule conflict' ' + test_create_repo directory-submodule && + ( + cd directory-submodule && + + git commit --allow-empty -m O && + + git branch A && + git branch B1 && + git branch B2 && + + git checkout B1 && + mkdir path && + echo contents >path/file && + git add path/file && + git commit -m B1 && + + git checkout B2 && + mkdir path && + echo contents >path/world && + git add path/world && + git commit -m B2 && + + git checkout A && + test_create_repo path && + ( + cd path && + + echo hello >world && + git add world && + git commit -m "submodule" + ) && + git add path && + git commit -m A + ) +' + +test_expect_failure 'directory/submodule conflict; keep submodule clean' ' + test_when_finished "git -C directory-submodule reset --hard" && + ( + cd directory-submodule && + + git checkout A^0 && + test_must_fail git merge B1^0 && + + git ls-files -s >out && + test_line_count = 2 out && + git ls-files -u >out && + test_line_count = 1 out && + + # path/ is still a submodule + test_path_is_dir path/.git && + + echo Checking if contents from B1:path/file showed up && + # Would rather use grep -r, but that is GNU extension... + git ls-files -co | xargs grep -q contents 2>/dev/null && + test_path_is_missing path/file + ) +' + +test_expect_failure 'directory/submodule conflict; should not treat submodule files as untracked or in the way' ' + test_when_finished "git -C directory-submodule/path reset --hard" && + test_when_finished "git -C directory-submodule reset --hard" && + ( + cd directory-submodule && + + git checkout A^0 && + test_must_fail git merge B2^0 >out 2>err && + + # We do not want files within the submodule to prevent the + # merge from starting; we should not be writing to such paths + # anyway. + test_i18ngrep ! "refusing to lose untracked file at" err + + ) +' + test_done -- 2.18.0.134.gafc206209.dirty
[PATCH 0/3] Add testcases showing suboptimal submodule/path conflict handling
Last November, Stefan asked me about an issue with submodules. In addition to providing a fix for that issue, I mentioned a few other problems I noticed with submodules and merging[1]. Turn those issues into testcases (as I probably should have done back then). [1] https://public-inbox.org/git/cabpp-bhdrw_daesic3xk7kc3jmgkenqupqf69opbvyhrkbh...@mail.gmail.com/ Elijah Newren (3): t7405: add a file/submodule conflict t7405: add a directory/submodule conflict t7405: verify 'merge --abort' works after submodule/path conflicts t/t7405-submodule-merge.sh | 173 + 1 file changed, 173 insertions(+) -- 2.18.0.134.gafc206209.dirty
[PATCH 1/3] t7405: add a file/submodule conflict
In the case of a file/submodule conflict, although both cannot exist at the same path, we expect both to be present somewhere for the user to be able to resolve the conflict with. Add a testcase for this. Signed-off-by: Elijah Newren --- t/t7405-submodule-merge.sh | 56 ++ 1 file changed, 56 insertions(+) diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh index 7bfb2f498..95fd05d83 100755 --- a/t/t7405-submodule-merge.sh +++ b/t/t7405-submodule-merge.sh @@ -279,4 +279,60 @@ test_expect_success 'recursive merge with submodule' ' grep "$(cat expect3)" actual > /dev/null) ' +# File/submodule conflict +# Commit O: +# Commit A: path (submodule) +# Commit B: path +# Expected: path/ is submodule and file contents for B's path are somewhere + +test_expect_success 'setup file/submodule conflict' ' + test_create_repo file-submodule && + ( + cd file-submodule && + + git commit --allow-empty -m O && + + git branch A && + git branch B && + + git checkout B && + echo contents >path && + git add path && + git commit -m B && + + git checkout A && + test_create_repo path && + ( + cd path && + + echo hello >world && + git add world && + git commit -m "submodule" + ) && + git add path && + git commit -m A + ) +' + +test_expect_failure 'file/submodule conflict' ' + test_when_finished "git -C file-submodule reset --hard" && + ( + cd file-submodule && + + git checkout A^0 && + test_must_fail git merge B^0 >out 2>err && + + git ls-files -s >out && + test_line_count = 2 out && + git ls-files -u >out && + test_line_count = 2 out && + + # path/ is still a submodule + test_path_is_dir path/.git && + + echo Checking if contents from B:path showed up anywhere && + grep -q content * 2>/dev/null + ) +' + test_done -- 2.18.0.134.gafc206209.dirty
[PATCH 3/3] t7405: verify 'merge --abort' works after submodule/path conflicts
Signed-off-by: Elijah Newren --- t/t7405-submodule-merge.sh | 26 ++ 1 file changed, 26 insertions(+) diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh index 6cb51c966..9f71a4859 100755 --- a/t/t7405-submodule-merge.sh +++ b/t/t7405-submodule-merge.sh @@ -335,6 +335,19 @@ test_expect_failure 'file/submodule conflict' ' ) ' +test_expect_success 'file/submodule conflict; merge --abort works afterward' ' + test_when_finished "git -C file-submodule reset --hard" && + ( + cd file-submodule && + + git checkout A^0 && + test_must_fail git merge B^0 >out 2>err && + + test_path_is_file .git/MERGE_HEAD && + git merge --abort + ) +' + # Directory/submodule conflict # Commit O: # Commit A: path (submodule), with sole tracked file named 'world' @@ -422,7 +435,20 @@ test_expect_failure 'directory/submodule conflict; should not treat submodule fi # merge from starting; we should not be writing to such paths # anyway. test_i18ngrep ! "refusing to lose untracked file at" err + ) +' + +test_expect_failure 'directory/submodule conflict; merge --abort works afterward' ' + test_when_finished "git -C directory-submodule/path reset --hard" && + test_when_finished "git -C directory-submodule reset --hard" && + ( + cd directory-submodule && + + git checkout A^0 && + test_must_fail git merge B2^0 >out 2>err && + test_path_is_file .git/MERGE_HEAD && + git merge --abort ) ' -- 2.18.0.134.gafc206209.dirty
Re: [RFC PATCH] Add 'human' date format
On Sat, Jul 7, 2018 at 12:58 PM Linus Torvalds
wrote:
>
> I'm playing with making all "today" dates just use the relative
> format.
Here's the incremental patch for that if people want to compare the output.
With this, you never get the "just time" case, because that will turn
into "2 hours ago" or similar. But you will get "Fri 19:45" for
something that happened yesterday.
So examples from my kernel logs look something like this:
2 hours ago
Fri 19:45
Fri 10:44 +1000
Fri Jun 22 15:46
Tue Jun 19 15:41 -0600
Thu Jun 15 12:57 2017 +0300
depending on how long ago they were and whether they were in the same
timezone etc.
Linus
date.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/date.c b/date.c
index 9809ac334..de0b03cf4 100644
--- a/date.c
+++ b/date.c
@@ -199,7 +199,7 @@ struct date_mode *date_mode_from_type(enum date_mode_type type)
return
}
-static void show_date_normal(struct strbuf *buf, struct tm *tm, int tz, struct tm *human_tm, int human_tz, int local)
+static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm, int tz, struct tm *human_tm, int human_tz, int local)
{
struct {
unsigned int year:1,
@@ -225,6 +225,14 @@ static void show_date_normal(struct strbuf *buf, struct tm *tm, int tz, struct t
}
}
+ /* Show "today" times as just relative times */
+ if (hide.wday) {
+ struct timeval now;
+ gettimeofday(, NULL);
+ show_date_relative(time, tz, , buf);
+ return;
+ }
+
/* Always hide seconds for human-readable */
hide.seconds = human_tm->tm_year > 0;
@@ -268,10 +276,6 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
/* Fill in the data for "current time" in human_tz and human_tm */
human_tz = local_time_tzoffset(now.tv_sec, _tm);
- /* Special case: if it's less than an hour ago, use relative time */
- if (time - now.tv_sec < 60 * 60)
- type = DATE_RELATIVE;
-
/* Don't print timezone if it matches */
if (tz == human_tz)
local = 1;
@@ -333,7 +337,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
strbuf_addftime(, mode->strftime_fmt, tm, tz,
!local);
else
- show_date_normal(, tm, tz, _tm, human_tz, local);
+ show_date_normal(, time, tm, tz, _tm, human_tz, local);
return timebuf.buf;
}
Re: [RFC PATCH] Add 'human' date format
On Sat, Jul 7, 2018 at 12:39 PM Linus Torvalds wrote: > to me, but with "--date=human", right now it just says > > Date: 12:21 Side note: this is probably my least favorite of the formats. I'm playing with making all "today" dates just use the relative format, and then the "a couple of days ago" dates would then have the > Date: Fri 19:45 format. But since it's _explicitly_ about a "human legible" format, I think the format could be a bit fluid, and things like that might be tweaked later. Anybody who would script this would be crazy. I'm more looking for "no, that's just stupid" comments, or "no, you're not the only one who has wanted this" kinds of replies. Linus
[RFC PATCH] Add 'human' date format
From: Linus Torvalds
This adds --date=human, which skips the timezone if it matches the
current time-zone, and doesn't print the whole date if that matches (ie
skip printing year for dates that are "this year", but also skip the
whole date itself if it's in the last few days and we can just say what
weekday it was).
For really recent dates (within the last hour), use the relative date
stamp.
Also add 'auto' date mode, which defaults to human if we're using the
pager. So you can do
git config --add log.date auto
and your "git log" commands will show the human-legible format unless
you're scripting things.
Signed-off-by: Linus Torvalds
---
Ok, I tried something like this long long ago, but that was a fairly nasty
patch that just defaulted to "--date=relative" for recent dates, and then
did the usual default for anything else.
But the issue kept bugging me, and this is a slightly more sane model (?).
It keeps the notion of "let's use --date=relative for very recent dates",
but for anything that is more than an hour old, it just uses a simplified
date.
For example, for time stamps that are "today", just show the time. And if
the year or the time zone matches the current year or timezone, skip them.
And don't bother showing seconds, because humans won't care.
The end result is a slightly simplified view.
So for example, the date for this commit right now looks like
Date: Sat Jul 7 12:21:26 2018 -0700
to me, but with "--date=human", right now it just says
Date: 12:21
(and maybe for a US locale, I should make it do the AM/PM thing).
This is marked RFC because
(a) maybe I'm the only one who has ever wanted the simplified dates
(b) the simplification rules themselves might be worthy of discussion.
For example, I also simplified the "a couple of days ago" case, so that it
shows
Date: Fri 19:45
for one of my kernel commits that happened yesterday. The date didn't
match _exactly_, but it's the same month, and just a few days ago, so it
just shows the weekday. Is that easier to read? Maybe. I kind of like it.
builtin/blame.c | 1 +
cache.h | 1 +
date.c | 139 +---
3 files changed, 110 insertions(+), 31 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 5a0388aae..27c64b1c8 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -917,6 +917,7 @@ int cmd_blame(int argc, const char **argv, const char
*prefix)
*/
blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) +
1; /* add the null */
break;
+ case DATE_HUMAN:
case DATE_NORMAL:
blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");
break;
diff --git a/cache.h b/cache.h
index d49092d94..8a6810ee6 100644
--- a/cache.h
+++ b/cache.h
@@ -1428,6 +1428,7 @@ extern struct object *peel_to_type(const char *name, int
namelen,
struct date_mode {
enum date_mode_type {
DATE_NORMAL = 0,
+ DATE_HUMAN,
DATE_RELATIVE,
DATE_SHORT,
DATE_ISO8601,
diff --git a/date.c b/date.c
index 49f943e25..9809ac334 100644
--- a/date.c
+++ b/date.c
@@ -77,22 +77,16 @@ static struct tm *time_to_tm_local(timestamp_t time)
}
/*
- * What value of "tz" was in effect back then at "time" in the
- * local timezone?
+ * Fill in the localtime 'struct tm' for the supplied time,
+ * and return the local tz.
*/
-static int local_tzoffset(timestamp_t time)
+static int local_time_tzoffset(time_t t, struct tm *tm)
{
- time_t t, t_local;
- struct tm tm;
+ time_t t_local;
int offset, eastwest;
- if (date_overflows(time))
- die("Timestamp too large for this system: %"PRItime, time);
-
- t = (time_t)time;
- localtime_r(, );
- t_local = tm_to_time_t();
-
+ localtime_r(, tm);
+ t_local = tm_to_time_t(tm);
if (t_local == -1)
return 0; /* error; just use + */
if (t_local < t) {
@@ -107,6 +101,20 @@ static int local_tzoffset(timestamp_t time)
return offset * eastwest;
}
+/*
+ * What value of "tz" was in effect back then at "time" in the
+ * local timezone?
+ */
+static int local_tzoffset(timestamp_t time)
+{
+ struct tm tm;
+
+ if (date_overflows(time))
+ die("Timestamp too large for this system: %"PRItime, time);
+
+ return local_time_tzoffset((time_t)time, );
+}
+
void show_date_relative(timestamp_t time, int tz,
const struct timeval *now,
struct strbuf *timebuf)
@@ -191,27 +199,94 @@ struct date_mode *date_mode_from_type(enum date_mode_type
type)
return
}
+static void show_date_normal(struct strbuf *buf, struct tm *tm, int tz, struct
tm *human_tm, int human_tz, int local)
+{
+ struct {
+ unsigned intyear:1,
+
Re: [PATCH v3 01/20] linear-assignment: a function to solve least-cost assignment problems
Hi Junio, On Sat, 7 Jul 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > >> Does the "gitgitgadget" thing lie on the Date: e-mail header? > > > > No, GitGitGadget takes the literal output from `git format-patch`, as far > > as I can tell. So if at all, it is `format-patch` that is lying. > > format-patch faithfully records the fact about the commit that is > made into the patch. How pieces of information should (or should > not) be used depends on the purpose of the application that uses > its output. I guess this is one of the fallouts for abusing the `format-patch|am` dance for `rebase--am`. > I'd suggest to match what send-email does, which is to notice but > use the current date when adding a Date: header. An option to lie > to SMTP servers may be OK but I do not think we want to encourage > such a behaviour by making it the default. I opened a PR to add a TODO: https://github.com/gitgitgadget/gitgitgadget/pull/15 > What is missing in the core-git tools is an ability to tell > send-email to optionaly add an in-body header to record the author > date of the original. We add an in-body header that records the > real author when it is different from the sender automatically, and > it is OK to have an option to allow doing so (but not encouraged > around here---it is easier to reason about the resulting history for > everybody, perhaps other than the original author, to record the > first time you show the change to the public as the author time). Pull Request-based workflows keep the original author date all the time. If that is not desired, we need to do more than paper over it by adjusting `send-email`. Ciao, Dscho
Re: [GSoC][PATCH v2 6/7] rebase -i: rewrite setup_reflog_action() in C
Junio C Hamano writes: > I actually think that is a good example of doing the same thing > slightly differently. ... [jc: Beating the dead horse, only to avoid misleading those who are learning from the sidelines...] The above was a stupid thing to say and end the message with, as it made it sound as if there are only two cases and we have a simple (but not that simple as alluded to earlier by Dscho that essentially says not to write 'else' when 'if' body returns or exits) rule that you should blindly apply after telling these two cases apart. That is a total opposite of the message that should be read from the whole thread.
Re: [PATCH v3 01/20] linear-assignment: a function to solve least-cost assignment problems
Johannes Schindelin writes: >> Does the "gitgitgadget" thing lie on the Date: e-mail header? > > No, GitGitGadget takes the literal output from `git format-patch`, as far > as I can tell. So if at all, it is `format-patch` that is lying. format-patch faithfully records the fact about the commit that is made into the patch. How pieces of information should (or should not) be used depends on the purpose of the application that uses its output. I'd suggest to match what send-email does, which is to notice but use the current date when adding a Date: header. An option to lie to SMTP servers may be OK but I do not think we want to encourage such a behaviour by making it the default. What is missing in the core-git tools is an ability to tell send-email to optionaly add an in-body header to record the author date of the original. We add an in-body header that records the real author when it is different from the sender automatically, and it is OK to have an option to allow doing so (but not encouraged around here---it is easier to reason about the resulting history for everybody, perhaps other than the original author, to record the first time you show the change to the public as the author time).
Re: [PATCH v3 4/4] builtin/rebase: support running "git rebase "
Christian Couder writes:
>> + default:
>> + BUG("Unhandled rebase type %d", opts->type);
>> + break;
>
> Nit: I think the "break;" line could be removed as the BUG() should always
> exit.
>
> A quick grep shows that there are other places where there is a
> "break;" line after a BUG() though. Maybe one of the #leftoverbits
> could be about removing those "break;" lines.
I do not see the above as nit, though. "could" is often quite a
different thing from "should", and in this case (and all the other
cases you incorrectly mark with %leftoverbits) removal of these
lines does not improve readability. In fact, if there were another
case arm after this one, having "break" there would help those who
scan the code, as the person who wants to ensure what that next case
arm does is correct is given a strong hint by "break;" immediately
before it that nothing in the previous case arm matters and does not
have to even be looked at. By removing it you force such a reader
to notice BUG() and reason that it does not matter because it does
not return control to us (hence no fallthru).
Re: What's (not) cooking
Hi Dscho,
On Sat, Jul 7, 2018 at 5:11 AM, Johannes Schindelin
wrote:
> Hi Elijah,
>
> On Fri, 6 Jul 2018, Elijah Newren wrote:
>
>> On Fri, Jul 6, 2018 at 3:57 PM, Junio C Hamano wrote:
>> > I'll be pushing out the integration branches with some updates, but
>> > there is no change in 'next' and below. The following topics I gave
>> > a quick look and gave them topic branches, but I had trouble merging
>> > them in 'pu' and making them work correctly or pass the tests, so
>> > they are not part of 'pu' in today's pushout.
>> >
>> > pk/rebase-in-c
>> > en/dirty-merge-fixes
>> > en/t6036-merge-recursive-tests
>> > en/t6042-insane-merge-rename-testcases
>> > ds/multi-pack-index
>>
>> It looks to me like the main problem is that pu itself has lots of
>> test failures. It seems to bisect down to
>> kg/gc-auto-windows-workaround. If I revert commit ac9d3fdbebbd ("gc
>> --auto: clear repository before auto packing", 2018-07-04), then pu
>> passes tests again for me.
>
> Is this the segmentation fault about which I just sent a mail?
Yes, this is is a gc segfault issue. It looks like every test calling
git gc will fail because of it, I even saw a "nothing to pack" message
or something like that followed by a segfault.
Re: What's (not) cooking
Hi Elijah,
On Fri, 6 Jul 2018, Elijah Newren wrote:
> On Fri, Jul 6, 2018 at 3:57 PM, Junio C Hamano wrote:
> > I'll be pushing out the integration branches with some updates, but
> > there is no change in 'next' and below. The following topics I gave
> > a quick look and gave them topic branches, but I had trouble merging
> > them in 'pu' and making them work correctly or pass the tests, so
> > they are not part of 'pu' in today's pushout.
> >
> > pk/rebase-in-c
> > en/dirty-merge-fixes
> > en/t6036-merge-recursive-tests
> > en/t6042-insane-merge-rename-testcases
> > ds/multi-pack-index
>
> It looks to me like the main problem is that pu itself has lots of
> test failures. It seems to bisect down to
> kg/gc-auto-windows-workaround. If I revert commit ac9d3fdbebbd ("gc
> --auto: clear repository before auto packing", 2018-07-04), then pu
> passes tests again for me.
Is this the segmentation fault about which I just sent a mail?
Ciao,
Dscho
Re: What's (not) cooking
Hi Junio, On Fri, 6 Jul 2018, Junio C Hamano wrote: > I'll be pushing out the integration branches with some updates, but > there is no change in 'next' and below. The following topics I gave > a quick look and gave them topic branches, but I had trouble merging > them in 'pu' and making them work correctly or pass the tests, so > they are not part of 'pu' in today's pushout. > > pk/rebase-in-c > en/dirty-merge-fixes > en/t6036-merge-recursive-tests > en/t6042-insane-merge-rename-testcases > ds/multi-pack-index Quick note that `pu` is broken on Windows: https://git-for-windows.visualstudio.com/git/git%20Team/_build/results?buildId=11901=logs One quite serious looking symptom is this (line 169728 in the log): 2018-07-07T00:25:33.9171932Z ./test-lib.sh: line 664: 4516 Segmentation fault git gc As you know, some time ago I tried to implement an automated build that auto-bisects issues like this, and unfortunately I had to disable this because it regularly ran out of time after 4h to bisect through the complex commit history (due to the fact that many branches in `pu` are not based on `master` but on commits that are way back in the past, and an automated build cannot retain information easily such as "this commit was clean, and please do not bother bisecting past it". And sadly, previous tests of `pu` (see https://git-for-windows.visualstudio.com/git/_build/index?definitionId=1&_a=history) were failing on Windows already in the compile stage, see e.g. https://git-for-windows.visualstudio.com/git/git%20Team/_build/results?buildId=11612=logs So I have no idea what caused this `gc` breakage. Ciao, Dscho
Re: [PATCH v3 4/4] builtin/rebase: support running "git rebase "
Hi Christian,
On Sat, 7 Jul 2018, Christian Couder wrote:
> On Fri, Jul 6, 2018 at 2:08 PM, Pratik Karki wrote:
>
> > + switch (opts->type) {
> > + case REBASE_AM:
> > + backend = "git-rebase--am";
> > + backend_func = "git_rebase__am";
> > + break;
> > + case REBASE_INTERACTIVE:
> > + backend = "git-rebase--interactive";
> > + backend_func = "git_rebase__interactive";
> > + break;
> > + case REBASE_MERGE:
> > + backend = "git-rebase--merge";
> > + backend_func = "git_rebase__merge";
> > + break;
> > + case REBASE_PRESERVE_MERGES:
> > + backend = "git-rebase--preserve-merges";
> > + backend_func = "git_rebase__preserve_merges";
> > + break;
> > + default:
> > + BUG("Unhandled rebase type %d", opts->type);
> > + break;
>
> Nit: I think the "break;" line could be removed as the BUG() should always
> exit.
>
> A quick grep shows that there are other places where there is a
> "break;" line after a BUG() though. Maybe one of the #leftoverbits
> could be about removing those "break;" lines.
But what if there is a bug in the BUG() function? Shouldn't we then not
call `die()` directly after the `BUG()`?
Okay, sorry, I let myself loose a little, as I think that we are still
safely in the territory where the code needs to be made correct. We can
nitpick when there are no "biggies" left to comment about. Maybe focus a
little more on whether the code does what it should do, rather than
whether some stylistic guidelines are violated?
I mean, we can argue back and forth about white-space, indentation,
superfluous break statements, etc. But that way, we won't get anywhere
with the builtin rebase.
Let's help Pratik instead to complete his project, okay?
Ciao,
Dscho
Re: [PATCH v3 18/20] completion: support `git range-diff`
Hi Junio, On Fri, 6 Jul 2018, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" > writes: > > > From: Johannes Schindelin > > > > Tab completion of `git range-diff` is very convenient, especially > > given that the revision arguments to specify the commit ranges to > > compare are typically more complex than, say, your grandfather's `git > > log` arguments. > > > > Signed-off-by: Johannes Schindelin > > Have three-dash lines here, or perhaps have some validation hook in > the garden-shears tool to notice these leftoer bits we see below? This is just a simple case of me overlooking the `squash!` while rebasing. The shears were not involved, as it is not a branch thicket, it is a simple, linear branch. I guess I could install some sort of post-rewrite hook, but then, I'd rather have this as a more generally useful feature, directly in `rebase -i`: if running in autosquash mode, when offering to edit squashed commit messages, `git rebase -i` should refuse by default to accept a commit message that contains a line that starts with `squash! `. Would make for a nice micro-project, methinks. > > squash! WIP completion: support `git range-diff` > > > > Revert "WIP completion: support `git range-diff`" > > > > This reverts commit 2e7af652af9e53a19fd947f8ebe37a78043afa49. > > --- I will fix this in my branch, of course, and force-push, but will wait with sending out a new revision of the patch series in case more reviews roll in. Ciao, Dscho
Re: [PATCH v3 01/20] linear-assignment: a function to solve least-cost assignment problems
Hi Junio, On Fri, 6 Jul 2018, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" > writes: > > > From: Johannes Schindelin > > > > The problem solved by the code introduced in this commit goes like this: > > given two sets of items, and a cost matrix which says how much it > > "costs" to assign any given item of the first set to any given item of > > the second, assign all items (except when the sets have different size) > > in the cheapest way. > > > > We use the Jonker-Volgenant algorithm to solve the assignment problem to > > answer questions such as: given two different versions of a topic branch > > (or iterations of a patch series), what is the best pairing of > > commits/patches between the different versions? > > > > Signed-off-by: Johannes Schindelin > > --- > > Does the "gitgitgadget" thing lie on the Date: e-mail header? No, GitGitGadget takes the literal output from `git format-patch`, as far as I can tell. So if at all, it is `format-patch` that is lying. You can compare the mail's date to the commit date: https://public-inbox.org/git/39272eefcfe66de3ca1aa2ee43d6626ce558caae.1530617166.git.gitgitgad...@gmail.com/ https://github.com/dscho/git/commit/39272eefcfe66de3ca1aa2ee43d6626ce558caae (the nice thing about GitGitGadget is that you can rely on its mails to reflect *precisely* what the commit is like, the user does not have any opportunity to interfere with the code that generates the mails: https://github.com/gitgitgadget/gitgitgadget/blob/c4805370f/lib/patch-series.ts#L605-L611). > Postdating the patch with in-body header is fine, but mailbox tools > often use and trust the Date: timestamp when sorting and finding > messages etc. so sending a new patch to add linear-assignment.c that > is different from what was added 9 weeks ago with "Date: Mon, 30 Apr > 2018" header can easily cause me to miss that message when I look > for things that happened within the past few weeks, for example. Well, isn't it too bad that we use emails to transport commits, then. Seriously, I have very little sympathy here, as all I am doing is to automate *the suggested usage* of `git format-patch` and `git send-email` (the latter of which I cannot even use due to its limitations). So if you want to see this "fixed", you should think how you want to see `git format-patch` fixed. Or maybe you want to write a script that re-orders the patches on top of the cover letter according to the `[PATCH M/N]` order, to reinstate the order of the original commits that got somewhat lost via emailing them. Of course, you could also save yourself a lot of trouble and use Git: git fetch https://github.com/gitgitgadget/git \ pr-1/dscho/branch-diff-v3 git cherry-pick -s ..FETCH_HEAD (This is assuming that you insist, as you did in the past, on changing the base commit from what the original author chose. If you are fine with my choice, which is the current `master`, then you could save yourself *even more* trouble by just pulling my branch, and merely signing off on the merge commit. Which would be totes okay with me.) Ciao, Dscho
Re: [PATCH v2] gc --auto: clear repository before auto packing
On Wed, Jul 4, 2018 at 10:16 PM Kim Gybels wrote: > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh > index e402aee6a2..ef599c11cd 100755 > --- a/t/t5510-fetch.sh > +++ b/t/t5510-fetch.sh > @@ -828,10 +828,12 @@ test_expect_success 'fetching with auto-gc does not > lock up' ' > test_commit test2 && > ( > cd auto-gc && > + git config fetch.unpackLimit 1 && > git config gc.autoPackLimit 1 && > git config gc.autoDetach false && > GIT_ASK_YESNO="$D/askyesno" git fetch >fetch.out 2>&1 && > - ! grep "Should I try again" fetch.out > + test_i18ngrep "Auto packing the repository" fetch.out && > + test_i18ngrep ! "Should I try again" fetch.out The messages containing "Auto packing the repository" are indeed translated, thus they have to be checked with 'test_i18ngrep', good. However, none of the "Should I try again" messages are translated, so checking them with bare 'grep' is fine and it shouldn't be changed. > ) > ' > > -- > 2.18.0.windows.1 >
Re: [PATCH] gc --auto: release pack files before auto packing
On Fri, Jul 6, 2018 at 6:39 PM Junio C Hamano wrote:
>
> Duy Nguyen writes:
>
> > On Sat, Jun 30, 2018 at 03:38:21PM +0200, Kim Gybels wrote:
> >> Teach gc --auto to release pack files before auto packing the repository
> >> to prevent failures when removing them.
> >>
> >> Also teach the test 'fetching with auto-gc does not lock up' to complain
> >> when it is no longer triggering an auto packing of the repository.
> >>
> >> Fixes https://github.com/git-for-windows/git/issues/500
> >>
> >> Signed-off-by: Kim Gybels
> >> ---
> >>
> >> Patch based on master, since problem doesn't reproduce on maint,
> >> however, the improvement to the test might be valuable on maint.
> >>
> >> builtin/gc.c | 1 +
> >> t/t5510-fetch.sh | 2 ++
> >> 2 files changed, 3 insertions(+)
> >>
> >> diff --git a/builtin/gc.c b/builtin/gc.c
> >> index ccfb1ceaeb..63b62ab57c 100644
> >> --- a/builtin/gc.c
> >> +++ b/builtin/gc.c
> >> @@ -612,6 +612,7 @@ int cmd_gc(int argc, const char **argv, const char
> >> *prefix)
> >> return -1;
> >>
> >> if (!repository_format_precious_objects) {
> >> +close_all_packs(the_repository->objects);
> >
> > We have repo_clear() which does this and potentially closing file
> > descriptors on other things as well. I suggest we use it, and before
> > any external command is run. Something like
> >
> > diff --git a/builtin/gc.c b/builtin/gc.c
> > index ccfb1ceaeb..a852c71da6 100644
> > --- a/builtin/gc.c
> > +++ b/builtin/gc.c
> > @@ -473,6 +473,13 @@ static int report_last_gc_error(void)
> >
> > static int gc_before_repack(void)
> > {
> > + /*
> > + * Shut down everything, we should have all the info we need
> > + * at this point. Leaving some file descriptors open may
> > + * prevent them from being removed on Windows.
> > + */
> > + repo_clear(the_repository);
> > +
> > if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
> > return error(FAILED_RUN, pack_refs_cmd.argv[0]);
>
> With
>
> https://public-inbox.org/git/20180509170409.13666-1-pclo...@gmail.com/
>
> the call to repo_clear() on the_repository itself may be safe [*1*],
> but if command line parsing code etc. has pointers to in-core object
> etc. obtained before this function was called, the codeflow after
> this function returns will be quite disturbed. Has anybody in this
> review thread audited the codeflow _after_ this function is run to
> make sure that invalidating in-core repository here does not cause
> us problems?
It does create us problems, unfortunately.
Among other things, a call to repo_clear(the_repository) does this:
raw_object_store_clear(repo->objects);
FREE_AND_NULL(repo->objects);
Later on cmd_gc() calls reprepare_packed_git(the_repository), which
then attempts to:
r->objects->approximate_object_count_valid = 0;
r->objects->packed_git_initialized = 0;
but with r->objects being NULL at this point that won't work, and in
turn causes failures in several test scripts.
FWIW, the original patch appears to be working fine, at least the test
suite passes.
Re: [PATCH v3 4/4] builtin/rebase: support running "git rebase "
On Fri, Jul 6, 2018 at 2:08 PM, Pratik Karki wrote:
> + switch (opts->type) {
> + case REBASE_AM:
> + backend = "git-rebase--am";
> + backend_func = "git_rebase__am";
> + break;
> + case REBASE_INTERACTIVE:
> + backend = "git-rebase--interactive";
> + backend_func = "git_rebase__interactive";
> + break;
> + case REBASE_MERGE:
> + backend = "git-rebase--merge";
> + backend_func = "git_rebase__merge";
> + break;
> + case REBASE_PRESERVE_MERGES:
> + backend = "git-rebase--preserve-merges";
> + backend_func = "git_rebase__preserve_merges";
> + break;
> + default:
> + BUG("Unhandled rebase type %d", opts->type);
> + break;
Nit: I think the "break;" line could be removed as the BUG() should always exit.
A quick grep shows that there are other places where there is a
"break;" line after a BUG() though. Maybe one of the #leftoverbits
could be about removing those "break;" lines.
> + }
