Re: Retrieving a file in git that was deleted and committed
On Fri, Dec 07, 2018 at 01:50:57PM -0800, biswaranjan panda wrote: > Thanks Jeff and Bryan! However, I am curious that if there were a way > to tell git blame to skip a commit (the one which added the file again > and maybe the one which deleted it originally) while it walks back > through history, then it should just get back the > entire history right ? Not easily. ;) You can feed a set of revisions to git-blame with the "-S" option, but I don't offhand know how it handles diffs (I think it would have to still diff each commit against its parent, since history is non-linear, and a list is inherently linear). You might want to experiment with that. Other than that, you can play with git-replace to produce a fake history, as if the deletion never happened. But note that will affect all commands, not just one particular blame. It might be a neat way to play with blame, but I doubt I'd leave the replacement in place in the long term. -Peff
Re: Difficulty with parsing colorized diff output
On Fri, Dec 07, 2018 at 07:09:58PM -0500, George King wrote: > My goal is to detect SGR color sequences, e.g. '\x1b[32m', that exist > in the source text, and have my highlighter print escaped > representations of those. For example, I have checked in files that > are expected test outputs for tools that emit color codes, and diffs > of those get very confusing. > > Figuring out which color codes are from the source text and which were > added by git is proving very difficult. The obvious solution is to > turn git diff coloring off, but as far as I can see this also turns > off all coloring for logs, which is undesirable. Another gotcha that I don't think you've hit yet: whitespace highlighting can color arbitrary parts of the line. Try this, for example: printf 'foo\n' >old printf '\t \tfoo\n' >new git diff --no-index old new So I'm not sure what you want to do can ever be completely robust. That said, I'm not opposed to making Git's color output more regular. It seems like a reasonable cleanup on its own. (For the Git project itself, we long ago realized that putting raw color codes into the source is a big pain when working with diffs, and we instead use tools like t/test-lib-functions.sh's test_decode_color). > * Context lines do not begin with reset code, but do end with a reset > code. It would be preferable in my opinion if they had both (like > every other line), or none at all. Context lines do have both. It's just that the default color for context lines is empty. ;) But yes, I think it might be reasonable to recognize when an opening color is empty, and turn the closing reset into a noop in that case (or I guess you are also advocating the opposite: turn an empty color into a noop \x1b[m or similar). I think most of the coloring, including context lines, is coming from diff.c:emit_diff_symbol_from_struct(). Instead of unconditionally calling: context = diff_get_color_opt(o, DIFF_CONTEXT); reset = diff_get_color_opt(o, DIFF_RESET); I suspect we could have a helper like this: static const char *diff_get_reset(const char *color) { if (!color || !*color) return ""; return diff_colors[DIFF_RESET]; } ... context = diff_get_color_opt(o, DIFF_CONTEXT); reset = diff_get_reset(context); > * Added lines have excess codes after the plus sign. The entire prefix > is, `\x1b[32m+\x1b[m\x1b[32m` translating to GREEN PLUS RESET GREEN. > Emitting codes after the plus sign makes the parsing more complex and > idiosyncratic. Yeah, I've run into this one, too (when working on diff-highlight, which similarly tries to pass-through Git's existing colors). It's unfortunately not quite trivial, due to some interactions with whitespace coloring. See this old patch: https://public-inbox.org/git/20101109220136.ga5...@sigill.intra.peff.net/ and the followup: https://public-inbox.org/git/20101118064050.ga12...@sigill.intra.peff.net/ > * Add a config feature to turn on log coloring while leaving diff coloring > off. Yes, the fact that --pretty log coloring is tied to diffs is mostly a historical artifact. I think it would be OK to introduce a color.log config option that defaults to the value of color.diff if unset. That would be backwards compatible, but allow you to set them independently if you wanted to. -Peff
Re: [PATCH 4/4] submodule deinit: unset core.worktree
Stefan Beller writes: > This re-introduces 984cd77ddb (submodule deinit: unset core.worktree, > 2018-06-18), which was reverted as part of f178c13fda (Revert "Merge > branch 'sb/submodule-core-worktree'", 2018-09-07) > > The whole series was reverted as the offending commit e98317508c > (submodule: ensure core.worktree is set after update, 2018-06-18) > was relied on by other commits such as 984cd77ddb. > > Keep the offending commit reverted, but its functionality came back via > 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17), such > that we can reintroduce 984cd77ddb now. None of the above three explains the most important thing directly, so readers fail to grasp what the main theme of the three-patch series is, without looking at the commits that were reverted already. Is the theme of the overall series to make sure core.worktree is set to point at the working tree when submodule's working tree is instantiated, and unset it when it is not? 2/4 was also explained (in the original) that it wants to unset and did so when "move_head" gets called. This one does the unset when a submodule is deinited. Are these the only two cases a submodule loses its working tree? If so, the log message for this step should declare victory by ending with something like ... as we covered the only other case in which a submodule loses its working tree in the earlier step (i.e. switching branches of top-level project to move to a commit that did not have the submodule), this makes the code always maintain core.worktree correctly unset when there is no working tree for a submodule. Thanks. I think I agree with what the series wants to do (if I read what it wants to do correctly, that is). > Signed-off-by: Stefan Beller > --- > builtin/submodule--helper.c | 2 ++ > t/lib-submodule-update.sh | 2 +- > t/t7400-submodule-basic.sh | 5 + > 3 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 31ac30cf2f..672b74db89 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -1131,6 +1131,8 @@ static void deinit_submodule(const char *path, const > char *prefix, > if (!(flags & OPT_QUIET)) > printf(format, displaypath); > > + submodule_unset_core_worktree(sub); > + > strbuf_release(_rm); > } > > diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh > index 51d449..5b56b23166 100755 > --- a/t/lib-submodule-update.sh > +++ b/t/lib-submodule-update.sh > @@ -235,7 +235,7 @@ reset_work_tree_to_interested () { > then > mkdir -p submodule_update/.git/modules/sub1/modules && > cp -r submodule_update_repo/.git/modules/sub1/modules/sub2 > submodule_update/.git/modules/sub1/modules/sub2 > - GIT_WORK_TREE=. git -C > submodule_update/.git/modules/sub1/modules/sub2 config --unset core.worktree > + # core.worktree is unset for sub2 as it is not checked out > fi && > # indicate we are interested in the submodule: > git -C submodule_update config submodule.sub1.url "bogus" && > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > index 76a7cb0af7..aba2d4d6ee 100755 > --- a/t/t7400-submodule-basic.sh > +++ b/t/t7400-submodule-basic.sh > @@ -984,6 +984,11 @@ test_expect_success 'submodule deinit should remove the > whole submodule section > rmdir init > ' > > +test_expect_success 'submodule deinit should unset core.worktree' ' > + test_path_is_file .git/modules/example/config && > + test_must_fail git config -f .git/modules/example/config core.worktree > +' > + > test_expect_success 'submodule deinit from subdirectory' ' > git submodule update --init && > git config submodule.example.foo bar &&
Re: [PATCH 3/4] submodule--helper: fix BUG message in ensure_core_worktree
Stefan Beller writes: > Shortly after f178c13fda (Revert "Merge branch > 'sb/submodule-core-worktree'", 2018-09-07), we had another series > that implemented partially the same, ensuring that core.worktree was > set in a checked out submodule, namely 74d4731da1 (submodule--helper: > replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13) > > As the series 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', > 2018-09-17) has different goals than the reverted series 7e25437d35 > (Merge branch 'sb/submodule-core-worktree', 2018-07-18), I'd wanted to > replay the series on top of it to reach the goal of having `core.worktree` > correctly set when the submodules worktree is present, and unset when the > worktree is not present. > > The replay resulted in a strange merge conflict highlighting that > the BUG message was not changed in 74d4731da1 (submodule--helper: > replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13). > > Fix the error message. > > Signed-off-by: Stefan Beller > --- Unlike the step 2/4 I commented on, this does explain what this wants to do and why, at least when looked from sideways. Is the above saying the same as the following two-liner? An ealier mistake while rebasing to produce 74d4731da1 failed to update this BUG message. Fix this. > builtin/submodule--helper.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index d38113a31a..31ac30cf2f 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2045,7 +2045,7 @@ static int ensure_core_worktree(int argc, const char > **argv, const char *prefix) > struct repository subrepo; > > if (argc != 2) > - BUG("submodule--helper connect-gitdir-workingtree > "); > + BUG("submodule--helper ensure-core-worktree "); > > path = argv[1];
Re: [PATCH] mailmap: update brandon williams's email address
On Fri, Dec 7, 2018 at 10:08 PM Junio C Hamano wrote: > > Stefan Beller writes: > > > On Fri, Dec 7, 2018 at 1:40 PM Jonathan Nieder wrote: > >> > >> Brandon Williams wrote: > >> > >> > Signed-off-by: Brandon Williams > >> > --- > >> > .mailmap | 1 + > >> > 1 file changed, 1 insertion(+) > >> > >> I can confirm that this is indeed the same person. > > > > What would be more of interest is why we'd be interested in this patch > > as there is no commit/patch sent by Brandon with this email in gits history. > > Once I "git am" the message that began this thread, there will be a > commit under this new ident, so that would be somewhat a moot point. > > If this were "Jonathan asked Brandon if we want to record an address > we can reach him in our .mailmap file and sent a patch to add one", > then the story is different, and I tend to agree with you that such > a patch is more or less pointless. That's not the purpose of the > mailmap file. > Turns out this is exactly the reason :) I've had a couple of people reach out to me asking me to do this because CCing my old email bounces and they've wanted my input/comments on something related to work I've done. If that's not the intended purpose then please ignore this patch > Not until git-send-email learns to use that file to rewrite > To/cc/etc to the "canonical" addresses, anyway ;-) > > I am not sure if there are people whose "canonical" address to be > used as the author is not necessarily the best address they want to > get their e-mails at, though. If we can be reasonably sure that the > set of such people is empty, then people can take the above mention > about send-email as a hint about a low-hanging fruit ;-) > > Thanks. > >
Re: [PATCH 2/4] submodule: unset core.worktree if no working tree is present
Stefan Beller writes: > This reintroduces 4fa4f90ccd (submodule: unset core.worktree if no working > tree is present, 2018-06-12), which was reverted as part of f178c13fda > (Revert "Merge branch 'sb/submodule-core-worktree'", 2018-09-07). > > 4fa4f90ccd was reverted as its followup commit was faulty, but without > the accompanying change of the followup, we'd have an incomplete workflow > of setting `core.worktree` again, when it is needed such as checking out > a revision that contains a submodule. > > So re-introduce that commit as-is, focusing on fixing up the followup I was hoping to hear (given what 0/4 claimed) a clearer explanation of what this change wants to achieve, but that is lacking. No need to grumble about an earlier work was that turned out to be inappropriate for the codebase back then. Repeatedly saying "this is needed" without giving further explaining why it is so, or anything like that, would help readers. Just pretend that the ealier commits and their reversion never happened, and further pretend that we are doing the best thing that should happen to our codebase. How would we explain this change, what the problem it tries to solve and what the solution looks like in the larger picture? When removing a working tree of a submodule (e.g. we may be switching back to an earlier commit in the superproject that did not have the submodule in question yet), we failed to unset core.worktree of the submodule's repository. That caused this and that issues, exhibited by a few new tests this commit adds. Make sure that core.worktree gets unset so that a leftover setting won't cause these issues. or something like that? I am just guessing by looking at the old commit's text, as the above two paragraphs and one sentence does not say much. > diff --git a/submodule.c b/submodule.c > index 6415cc5580..d393e947e6 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -1561,6 +1561,18 @@ int bad_to_remove_submodule(const char *path, unsigned > flags) > return ret; > } > > +void submodule_unset_core_worktree(const struct submodule *sub) > +{ > + char *config_path = xstrfmt("%s/modules/%s/config", > + get_git_common_dir(), sub->name); > + > + if (git_config_set_in_file_gently(config_path, "core.worktree", NULL)) > + warning(_("Could not unset core.worktree setting in submodule > '%s'"), > + sub->path); > + > + free(config_path); > +} > + > static const char *get_super_prefix_or_empty(void) > { > const char *s = get_super_prefix(); > @@ -1726,6 +1738,8 @@ int submodule_move_head(const char *path, > > if (is_empty_dir(path)) > rmdir_or_warn(path); > + > + submodule_unset_core_worktree(sub); > } > } > out: > diff --git a/submodule.h b/submodule.h > index a680214c01..9e18e9b807 100644 > --- a/submodule.h > +++ b/submodule.h > @@ -131,6 +131,8 @@ int submodule_move_head(const char *path, > const char *new_head, > unsigned flags); > > +void submodule_unset_core_worktree(const struct submodule *sub); > + > /* > * Prepare the "env_array" parameter of a "struct child_process" for > executing > * a submodule by clearing any repo-specific environment variables, but > diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh > index 016391723c..51d449 100755 > --- a/t/lib-submodule-update.sh > +++ b/t/lib-submodule-update.sh > @@ -709,7 +709,8 @@ test_submodule_recursing_with_args_common() { > git branch -t remove_sub1 origin/remove_sub1 && > $command remove_sub1 && > test_superproject_content origin/remove_sub1 && > - ! test -e sub1 > + ! test -e sub1 && > + test_must_fail git config -f .git/modules/sub1/config > core.worktree > ) > ' > # ... absorbing a .git directory along the way.
Re: [RFC PATCH 2/3] Documentation/git-rev-list: s///
Matthew DeVore writes: > $ git -C pc1 fetch-pack --stdin "file://$(pwd)/srv.bare" > Where observed.oids contains all the blobs that were missing. It tells > the remote that it already has the "refs/heads/master" commit, which > means it is excluded. Before, this worked fine, since it didn't mean > the blobs were excluded, only the commit itself. So 'master' has some blob in its tree, which is missing from the repository, in this test? Which means that such a repository is "corrupt" and does not pass connectivity check by fsck. I am of two minds. If we claim that we have everything that is needed to complete the commit sitting at the tip of 'master', I think it is correct for the other side not to send a blob that is in 'master' (or its ancestors), so your "fix" may (at least technically) be more correct than the status quo. On the other hand, if possession of commit 'master' does not defeat an explicit request for a blob in it, that would actually be a good thing---it would be a very straight-forward way to recover from such form of repository corruption. Fetching isolated objects without walking is also something that would help backfill a lazily created clone, and I even vaguely recall an effort to allow objects explicitly requested to be always fetched regardless of the connectivity, if I am not mistaken (JTan?) Anyway, thanks for a thoughtful response.
Re: [PATCH] mailmap: update brandon williams's email address
Stefan Beller writes: > On Fri, Dec 7, 2018 at 1:40 PM Jonathan Nieder wrote: >> >> Brandon Williams wrote: >> >> > Signed-off-by: Brandon Williams >> > --- >> > .mailmap | 1 + >> > 1 file changed, 1 insertion(+) >> >> I can confirm that this is indeed the same person. > > What would be more of interest is why we'd be interested in this patch > as there is no commit/patch sent by Brandon with this email in gits history. Once I "git am" the message that began this thread, there will be a commit under this new ident, so that would be somewhat a moot point. If this were "Jonathan asked Brandon if we want to record an address we can reach him in our .mailmap file and sent a patch to add one", then the story is different, and I tend to agree with you that such a patch is more or less pointless. That's not the purpose of the mailmap file. Not until git-send-email learns to use that file to rewrite To/cc/etc to the "canonical" addresses, anyway ;-) I am not sure if there are people whose "canonical" address to be used as the author is not necessarily the best address they want to get their e-mails at, though. If we can be reasonably sure that the set of such people is empty, then people can take the above mention about send-email as a hint about a low-hanging fruit ;-) Thanks.
Re: RFE: git-patch-id should handle patches without leading "diff"
Jonathan Nieder writes: >> So it seems most sensible to me if this is going to be supported that we >> go a bit beyond the call of duty and fake up the start of it, namely: >> >> --- a/arch/x86/kernel/process.c >> +++ b/arch/x86/kernel/process.c >> >> To be: >> >> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c >> --- a/arch/x86/kernel/process.c >> +++ b/arch/x86/kernel/process.c > > Right. We may want to handle diff.mnemonicPrefix as well. I definitely think under the --stable option, we should pretend as if the canonical a/ vs b/ prefixes were given with the "diff --git" header, just like we try to reverse the effect of diff-orderfile, etc. I am unsure what the right behaviour under --unstable is, though.
Re: [PATCH 0/4]
Stefan Beller writes: > A couple days before the 2.19 release we had a bug report about > broken submodules[1] and reverted[2] the commits leading up to them. > > The behavior of said bug fixed itself by taking a different approach[3], > specifically by a weaker enforcement of having `core.worktree` set in a > submodule [4]. > > The revert [2] was overly broad as we neared the release, such that we wanted > to rather keep the known buggy behavior of always having `core.worktree` set, > rather than figuring out how to fix the new bug of having 'git submodule > update' > not working in old style repository setups. > > This series re-introduces those reverted patches, with no changes in code, > but with drastically changed commit messages, as those focus on why it is safe > to re-introduce them instead of explaining the desire for the change. The above was a bit too cryptic for me to grok, so let me try rephrasing to see if I got them all correctly. - three-patch series leading to 984cd77ddb were meant to fix some bug, but the series itself was buggy and caused problems; we got rid of them - the problem 984cd77ddb wanted to fix was fixed differently without reintroducing the problem three-patch series introduced. That fix is already with us since 4d6d6ef1fc. - now these three changes that were problematic in the past is resent without any update (other than that it has one preparatory patch to add tests). Is that what is going on? Obviously I am not getting "the other" benefit we wanted to gain out of these three patches (because the above description fails to explain what that is), other than to fix the issue that was fixed by 4d6d6ef1fc. Sorry for being puzzled... > [1] https://public-inbox.org/git/2659750.rG6xLiZASK@twilight > [2] f178c13fda (Revert "Merge branch 'sb/submodule-core-worktree'", > 2018-09-07) > [3] 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17) > [4] 74d4731da1 (submodule--helper: replace connect-gitdir-workingtree by > ensure-core-worktree, 2018-08-13) > > Stefan Beller (4): > submodule update: add regression test with old style setups > submodule: unset core.worktree if no working tree is present > submodule--helper: fix BUG message in ensure_core_worktree > submodule deinit: unset core.worktree > > builtin/submodule--helper.c| 4 +++- > submodule.c| 14 ++ > submodule.h| 2 ++ > t/lib-submodule-update.sh | 5 +++-- > t/t7400-submodule-basic.sh | 5 + > t/t7412-submodule-absorbgitdirs.sh | 7 ++- > 6 files changed, 33 insertions(+), 4 deletions(-)
Re: [PATCH] commit: abort before commit-msg if empty message
Jonathan Tan writes: > When a user runs "git commit" without specifying a message, an editor > appears with advice: > > Please enter the commit message for your changes. Lines starting > with '#' will be ignored, and an empty message aborts the commit. > > However, if the user supplies an empty message and has a commit-msg hook > which updates the message to be non-empty, the commit proceeds to occur, > despite what the advice states. When "--no-edit" is given, and when commit-msg fills that blank, the command should go ahead and record the commit, I think. An automation where commit-msg is used to produce whatever appropriate message for the automation is entirely a reasonable thing to arrange. Of course, you can move the logic to produce an appropriate message for the automation from commit-msg to the script that drives the "git commit" and use the output of that logic as the value for the "-m" option to achieve the same, so in that sense, there is an escape hatch even if you suddenly start to forbid such a working set-up, but it nevertheless is unnecessary busywork for those with such a set-up to adjust to this change. I actually think in this partcular case, the commit-msg hook that adds Change-ID to an empty message is BUGGY. If the hook looked at the message contents and refrains from making an otherwise empty message to non-empty, there is no need for any change here. In any case, you'll have plenty of time to make your case after the rc freeze. I am not so sympathetic to a patch that makes us bend backwards to support such a buggy hook to e honest.
Re: [wishlist] git submodule update --reset-hard
On Fri, 07 Dec 2018, Yaroslav Halchenko wrote: > On Fri, 07 Dec 2018, Stefan Beller wrote: > > > the initial "git submodule update --reset-hard" is pretty much a > > > crude workaround for some of those cases, so I would just go earlier in > > > the history, and redo some things, whenever I could just drop or revert > > > some selected set of commits. > > That makes sense. > > Do you want to give the implementation a try for the --reset-hard switch? > ok, will do, thanks for the blessing ;-) The patch is attached (please advise if should be done differently) and also submitted as PR https://github.com/git/git/pull/563 I guess it would need more tests. Took me some time to figure out why I was getting fatal: bad value for update parameter after all my changes to the git-submodule.sh script after looking at an example commit 42b491786260eb17d97ea9fb1c4b70075bca9523 which introduced --merge to the update ;-) -- Yaroslav O. Halchenko Center for Open Neuroscience http://centerforopenneuroscience.org Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755 Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419 WWW: http://www.linkedin.com/in/yarik From 170296dc661b4bc3d942917ce27288df52ff650d Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Fri, 7 Dec 2018 21:28:29 -0500 Subject: [PATCH] submodule: Add --reset-hard option for git submodule update This patch adds a --reset-hard option for the update command to hard reset submodule(s) to the gitlink for the corresponding submodule in the superproject. This feature is desired e.g. to be able to discard recent changes in the entire hierarchy of the submodules after running git reset --hard PREVIOUS_STATE in the superproject which leaves submodules in their original state, and git reset --hard --recurse-submodules PREVIOUS_STATE would result in the submodules being checked into detached HEADs. As in the original git reset --hard no checks or any kind of safe-guards against jumping into some state which was never on the current branch is done. must_die_on_failure is not set to yes to mimic behavior of a update --checkout strategy, which would leave user with a non-clean state immediately apparent via git status so an informed decision/actions could be done manually. Signed-off-by: Yaroslav Halchenko --- Documentation/git-submodule.txt | 12 +++- Documentation/gitmodules.txt| 4 ++-- builtin/submodule--helper.c | 3 ++- git-submodule.sh| 10 +- submodule.c | 4 submodule.h | 1 + t/t7406-submodule-update.sh | 17 - 7 files changed, 45 insertions(+), 6 deletions(-) diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt index ba3c4df550..f90a42d265 100644 --- a/Documentation/git-submodule.txt +++ b/Documentation/git-submodule.txt @@ -124,7 +124,7 @@ If you really want to remove a submodule from the repository and commit that use linkgit:git-rm[1] instead. See linkgit:gitsubmodules[7] for removal options. -update [--init] [--remote] [-N|--no-fetch] [--[no-]recommend-shallow] [-f|--force] [--checkout|--rebase|--merge] [--reference ] [--depth ] [--recursive] [--jobs ] [--] [...]:: +update [--init] [--remote] [-N|--no-fetch] [--[no-]recommend-shallow] [-f|--force] [--checkout|--rebase|--merge|--reset-hard] [--reference ] [--depth ] [--recursive] [--jobs ] [--] [...]:: + -- Update the registered submodules to match what the superproject @@ -358,6 +358,16 @@ the submodule itself. If the key `submodule.$name.update` is set to `rebase`, this option is implicit. +--reset-hard:: + This option is only valid for the update command. + Hard reset current state to the commit recorded in the superproject. +If this option is given, the submodule's HEAD will not get detached +if it was not detached before. Note that, like with a regular +git reset --hard no safe-guards are in place to prevent jumping +to a commit which was never part of the current branch. + If the key `submodule.$name.update` is set to `reset-hard`, this + option is implicit. + --init:: This option is only valid for the update command. Initialize all submodules for which "git submodule init" has not been diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt index 312b6f9259..e085dbc01f 100644 --- a/Documentation/gitmodules.txt +++ b/Documentation/gitmodules.txt @@ -43,8 +43,8 @@ submodule..update:: command in the superproject. This is only used by `git submodule init` to initialize the configuration variable of the same name. Allowed values here are 'checkout', 'rebase', - 'merge' or 'none'. See description of 'update' command in - linkgit:git-submodule[1] for their meaning. Note that the + 'merge', 'reset-hard' or 'none'. See description of 'update' command + in linkgit:git-submodule[1] for their meaning. Note that the '!command' form
Re: bug: git fetch reports too many unreachable loose objects
On Fri, Dec 7, 2018 at 6:14 PM Josh Wolfe wrote: > > git version 2.19.1 > steps to reproduce: > > # start in a brand new repo > git init > > # create lots of unreachable loose objects > for i in {1..1}; do git commit-tree -m "$(head -c 12 /dev/urandom > | base64)" "$(git mktree <&-)" <&-; done > # this prints a lot of output and takes a minute or so to run > > # trigger git gc to run in the background > git fetch > # Auto packing the repository in background for optimum performance. > # See "git help gc" for manual housekeeping. > > # trigger it again > git fetch > # Auto packing the repository in background for optimum performance. > # See "git help gc" for manual housekeeping. > # error: The last gc run reported the following. Please correct the root cause > # and remove .git/gc.log. > # Automatic cleanup will not be performed until the file is removed. > # > # warning: There are too many unreachable loose objects; run 'git > prune' to remove them. > > # to manually fix this, run git prune: > git prune > > # note that `git gc` does not fix the problem, and appears to do > nothing in this situation: > git gc > > > According to the `git fetch` output, the `git help gc` docs, and the > `git help prune` docs, I don't think I shouldn't ever have to run `git > prune` manually, so this behavior seems like a bug to me. Please > correct me if this is expected behavior. Known bug, there are a variety of other ways to trigger it too. See the threads here for more info: https://public-inbox.org/git/87inc89j38@evledraar.gmail.com/ https://public-inbox.org/git/20180716172717.237373-1-jonathanta...@google.com/ There are probably other threads as well.
Re: [wishlist] git submodule update --reset-hard
On Fri, 07 Dec 2018, Stefan Beller wrote: > > the initial "git submodule update --reset-hard" is pretty much a > > crude workaround for some of those cases, so I would just go earlier in > > the history, and redo some things, whenever I could just drop or revert > > some selected set of commits. > That makes sense. > Do you want to give the implementation a try for the --reset-hard switch? ok, will do, thanks for the blessing ;-) -- Yaroslav O. Halchenko Center for Open Neuroscience http://centerforopenneuroscience.org Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755 Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419 WWW: http://www.linkedin.com/in/yarik
bug: git fetch reports too many unreachable loose objects
git version 2.19.1 steps to reproduce: # start in a brand new repo git init # create lots of unreachable loose objects for i in {1..1}; do git commit-tree -m "$(head -c 12 /dev/urandom | base64)" "$(git mktree <&-)" <&-; done # this prints a lot of output and takes a minute or so to run # trigger git gc to run in the background git fetch # Auto packing the repository in background for optimum performance. # See "git help gc" for manual housekeeping. # trigger it again git fetch # Auto packing the repository in background for optimum performance. # See "git help gc" for manual housekeeping. # error: The last gc run reported the following. Please correct the root cause # and remove .git/gc.log. # Automatic cleanup will not be performed until the file is removed. # # warning: There are too many unreachable loose objects; run 'git prune' to remove them. # to manually fix this, run git prune: git prune # note that `git gc` does not fix the problem, and appears to do nothing in this situation: git gc According to the `git fetch` output, the `git help gc` docs, and the `git help prune` docs, I don't think I shouldn't ever have to run `git prune` manually, so this behavior seems like a bug to me. Please correct me if this is expected behavior. In case anyone's wondering why I'm creating unreachable loose objects, here's the usecase: https://stackoverflow.com/a/50403179/367916 . I would love a first-class solution to obviate that workaround, but that is probably a separate issue. Josh
Re: [PATCH v2 1/3] git clone C:\cygwin\home\USER\repo' is working (again)
On Fri, Dec 7, 2018 at 11:04 AM wrote: > The solution is to implement has_dos_drive_prefix(), skip_dos_drive_prefix() > is_dir_sep(), offset_1st_component() and convert_slashes() for cygwin > in the same way as it is done in 'Git for Windows' in compat/mingw.[ch] > > Instead of duplicating the code, it is extracted into compat/mingw-cygwin.[ch] > Some need for refactoring and cleanup came up in the review, they are adressed > in a seperate commit. i have applied the 3 patches against current master, and my original test passes, so looks good to me. however like Johannes i take issue with the naming. as he said "mingw-cygwin" really isnt appropriate. ideally it would be "windows.h", but as that is conspicuously in use, something like these: - pc-windows - pc-win - win i disagree with him on using "win32" - that doesnt really make sense, as obviously you can compile 64-bit Git for Windows. if you wanted to go that route you would want to use something like: - windows-api - win-api - winapi further - i disagree with the "DOS" moniker being used at all. DOS is a defunkt operating system that i dont think Git has *ever* supported, so it doesnt make sense to be referring to it this way. again, a more approriate name would be something like "win_drive_prefix".
Re: [PATCH] docs/gitweb.conf: config variable typo
On Fri, Dec 7, 2018 at 7:45 PM Jonathan Nieder wrote: > > > Thanks for fixing it. May we forge your sign-off? Yes please, guess I didn't read far enough down that document. My apologies. Consider the previous patch email: Signed-off-by: FeRD (Frank Dana)
Re: [PATCH] docs/gitweb.conf: config variable typo
Hi, Frank Dana wrote: > The documentation for the feature 'snapshot' claimed > "This feature can be configured on a per-repository basis via > repository's `gitweb.blame` configuration variable" > > Fixed to specify `gitweb.snapshot` as the variable name. > --- > Documentation/gitweb.conf.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks for fixing it. May we forge your sign-off? See Documentation/SubmittingPatches section "Certify your work" for what this means. Sincerely, Jonathan
[PATCH] docs/gitweb.conf: config variable typo
The documentation for the feature 'snapshot' claimed "This feature can be configured on a per-repository basis via repository's `gitweb.blame` configuration variable" Fixed to specify `gitweb.snapshot` as the variable name. --- Documentation/gitweb.conf.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt index c0a326e3883c3..40c9563ef67af 100644 --- a/Documentation/gitweb.conf.txt +++ b/Documentation/gitweb.conf.txt @@ -684,7 +684,7 @@ compressed tar archive) and "zip"; please consult gitweb sources for a definitive list. By default only "tgz" is offered. + This feature can be configured on a per-repository basis via -repository's `gitweb.blame` configuration variable, which contains +repository's `gitweb.snapshot` configuration variable, which contains a comma separated list of formats or "none" to disable snapshots. Unknown values are ignored. -- https://github.com/git/git/pull/562
Difficulty with parsing colorized diff output
Hello, I have a rather elaborate diff highlighter that I have implemented as a post-processor to regular git output. I am writing to discuss some difficult aspects of git diff's color output that I am observing with version 2.19.2. This is not a regression report; I am trying to implement a new feature and am stymied by these details. My goal is to detect SGR color sequences, e.g. '\x1b[32m', that exist in the source text, and have my highlighter print escaped representations of those. For example, I have checked in files that are expected test outputs for tools that emit color codes, and diffs of those get very confusing. Figuring out which color codes are from the source text and which were added by git is proving very difficult. The obvious solution is to turn git diff coloring off, but as far as I can see this also turns off all coloring for logs, which is undesirable. Then I tried to remove just the color codes that git adds to the diff. This almost works, but there are some irregularities. Most lines begin with a style/color code and end with a reset code, which would be a perfect indicator that git is using colors. However: * Context lines do not begin with reset code, but do end with a reset code. It would be preferable in my opinion if they had both (like every other line), or none at all. * Added lines have excess codes after the plus sign. The entire prefix is, `\x1b[32m+\x1b[m\x1b[32m` translating to GREEN PLUS RESET GREEN. Emitting codes after the plus sign makes the parsing more complex and idiosyncratic. In summary, I would like to suggest the following improvements: * Remove the excess codes after the plus sign. * When git diff is adding colors, ensure that every line begins with an SGR code and ends with the RESET code. * Add a config feature to turn on log coloring while leaving diff coloring off. I would be willing to attempt a fix for this myself, but I'd like to hear what the maintainers think first, and would appreciate any hints as to where I should start looking in the code base. If anyone is curious about the implementation it is called `same-same` and lives here: https://github.com/gwk/pithy/blob/master/pithy/bin/same_same.py I configure it like this in .gitconfig: [core] pager = same-same | LESSANSIENDCHARS=mK less --RAW-CONTROL-CHARS [interactive] diffFilter = same-same -interactive | LESSANSIENDCHARS=mK less --RAW-CONTROL-CHARS Thank you, George
[PATCH 1/4] submodule update: add regression test with old style setups
As f178c13fda (Revert "Merge branch 'sb/submodule-core-worktree'", 2018-09-07) was produced shortly before a release, nobody asked for a regression test to be included. Add a regression test that makes sure that the invocation of `git submodule update` on old setups doesn't produce errors as pointed out in f178c13fda. The place to add such a regression test may look odd in t7412, but that is the best place as there we setup old style submodule setups explicitly. Signed-off-by: Stefan Beller --- t/t7412-submodule-absorbgitdirs.sh | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh index ce74c12da2..1cfa150768 100755 --- a/t/t7412-submodule-absorbgitdirs.sh +++ b/t/t7412-submodule-absorbgitdirs.sh @@ -75,7 +75,12 @@ test_expect_success 're-setup nested submodule' ' GIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \ core.worktree "../../../nested" && # make sure this re-setup is correct - git status --ignore-submodules=none + git status --ignore-submodules=none && + + # also make sure this old setup does not regress + git submodule update --init --recursive >out 2>err && + test_must_be_empty out && + test_must_be_empty err ' test_expect_success 'absorb the git dir in a nested submodule' ' -- 2.20.0.rc2.403.gdbc3b29805-goog
[PATCH 4/4] submodule deinit: unset core.worktree
This re-introduces 984cd77ddb (submodule deinit: unset core.worktree, 2018-06-18), which was reverted as part of f178c13fda (Revert "Merge branch 'sb/submodule-core-worktree'", 2018-09-07) The whole series was reverted as the offending commit e98317508c (submodule: ensure core.worktree is set after update, 2018-06-18) was relied on by other commits such as 984cd77ddb. Keep the offending commit reverted, but its functionality came back via 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17), such that we can reintroduce 984cd77ddb now. Signed-off-by: Stefan Beller --- builtin/submodule--helper.c | 2 ++ t/lib-submodule-update.sh | 2 +- t/t7400-submodule-basic.sh | 5 + 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 31ac30cf2f..672b74db89 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1131,6 +1131,8 @@ static void deinit_submodule(const char *path, const char *prefix, if (!(flags & OPT_QUIET)) printf(format, displaypath); + submodule_unset_core_worktree(sub); + strbuf_release(_rm); } diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index 51d449..5b56b23166 100755 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -235,7 +235,7 @@ reset_work_tree_to_interested () { then mkdir -p submodule_update/.git/modules/sub1/modules && cp -r submodule_update_repo/.git/modules/sub1/modules/sub2 submodule_update/.git/modules/sub1/modules/sub2 - GIT_WORK_TREE=. git -C submodule_update/.git/modules/sub1/modules/sub2 config --unset core.worktree + # core.worktree is unset for sub2 as it is not checked out fi && # indicate we are interested in the submodule: git -C submodule_update config submodule.sub1.url "bogus" && diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index 76a7cb0af7..aba2d4d6ee 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -984,6 +984,11 @@ test_expect_success 'submodule deinit should remove the whole submodule section rmdir init ' +test_expect_success 'submodule deinit should unset core.worktree' ' + test_path_is_file .git/modules/example/config && + test_must_fail git config -f .git/modules/example/config core.worktree +' + test_expect_success 'submodule deinit from subdirectory' ' git submodule update --init && git config submodule.example.foo bar && -- 2.20.0.rc2.403.gdbc3b29805-goog
[PATCH 0/4]
A couple days before the 2.19 release we had a bug report about broken submodules[1] and reverted[2] the commits leading up to them. The behavior of said bug fixed itself by taking a different approach[3], specifically by a weaker enforcement of having `core.worktree` set in a submodule [4]. The revert [2] was overly broad as we neared the release, such that we wanted to rather keep the known buggy behavior of always having `core.worktree` set, rather than figuring out how to fix the new bug of having 'git submodule update' not working in old style repository setups. This series re-introduces those reverted patches, with no changes in code, but with drastically changed commit messages, as those focus on why it is safe to re-introduce them instead of explaining the desire for the change. [1] https://public-inbox.org/git/2659750.rG6xLiZASK@twilight [2] f178c13fda (Revert "Merge branch 'sb/submodule-core-worktree'", 2018-09-07) [3] 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17) [4] 74d4731da1 (submodule--helper: replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13) Stefan Beller (4): submodule update: add regression test with old style setups submodule: unset core.worktree if no working tree is present submodule--helper: fix BUG message in ensure_core_worktree submodule deinit: unset core.worktree builtin/submodule--helper.c| 4 +++- submodule.c| 14 ++ submodule.h| 2 ++ t/lib-submodule-update.sh | 5 +++-- t/t7400-submodule-basic.sh | 5 + t/t7412-submodule-absorbgitdirs.sh | 7 ++- 6 files changed, 33 insertions(+), 4 deletions(-) -- 2.20.0.rc2.403.gdbc3b29805-goog
[PATCH 2/4] submodule: unset core.worktree if no working tree is present
This reintroduces 4fa4f90ccd (submodule: unset core.worktree if no working tree is present, 2018-06-12), which was reverted as part of f178c13fda (Revert "Merge branch 'sb/submodule-core-worktree'", 2018-09-07). 4fa4f90ccd was reverted as its followup commit was faulty, but without the accompanying change of the followup, we'd have an incomplete workflow of setting `core.worktree` again, when it is needed such as checking out a revision that contains a submodule. So re-introduce that commit as-is, focusing on fixing up the followup Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano Signed-off-by: Stefan Beller --- submodule.c | 14 ++ submodule.h | 2 ++ t/lib-submodule-update.sh | 3 ++- 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/submodule.c b/submodule.c index 6415cc5580..d393e947e6 100644 --- a/submodule.c +++ b/submodule.c @@ -1561,6 +1561,18 @@ int bad_to_remove_submodule(const char *path, unsigned flags) return ret; } +void submodule_unset_core_worktree(const struct submodule *sub) +{ + char *config_path = xstrfmt("%s/modules/%s/config", + get_git_common_dir(), sub->name); + + if (git_config_set_in_file_gently(config_path, "core.worktree", NULL)) + warning(_("Could not unset core.worktree setting in submodule '%s'"), + sub->path); + + free(config_path); +} + static const char *get_super_prefix_or_empty(void) { const char *s = get_super_prefix(); @@ -1726,6 +1738,8 @@ int submodule_move_head(const char *path, if (is_empty_dir(path)) rmdir_or_warn(path); + + submodule_unset_core_worktree(sub); } } out: diff --git a/submodule.h b/submodule.h index a680214c01..9e18e9b807 100644 --- a/submodule.h +++ b/submodule.h @@ -131,6 +131,8 @@ int submodule_move_head(const char *path, const char *new_head, unsigned flags); +void submodule_unset_core_worktree(const struct submodule *sub); + /* * Prepare the "env_array" parameter of a "struct child_process" for executing * a submodule by clearing any repo-specific environment variables, but diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index 016391723c..51d449 100755 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -709,7 +709,8 @@ test_submodule_recursing_with_args_common() { git branch -t remove_sub1 origin/remove_sub1 && $command remove_sub1 && test_superproject_content origin/remove_sub1 && - ! test -e sub1 + ! test -e sub1 && + test_must_fail git config -f .git/modules/sub1/config core.worktree ) ' # ... absorbing a .git directory along the way. -- 2.20.0.rc2.403.gdbc3b29805-goog
[PATCH 3/4] submodule--helper: fix BUG message in ensure_core_worktree
Shortly after f178c13fda (Revert "Merge branch 'sb/submodule-core-worktree'", 2018-09-07), we had another series that implemented partially the same, ensuring that core.worktree was set in a checked out submodule, namely 74d4731da1 (submodule--helper: replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13) As the series 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17) has different goals than the reverted series 7e25437d35 (Merge branch 'sb/submodule-core-worktree', 2018-07-18), I'd wanted to replay the series on top of it to reach the goal of having `core.worktree` correctly set when the submodules worktree is present, and unset when the worktree is not present. The replay resulted in a strange merge conflict highlighting that the BUG message was not changed in 74d4731da1 (submodule--helper: replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13). Fix the error message. Signed-off-by: Stefan Beller --- builtin/submodule--helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index d38113a31a..31ac30cf2f 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2045,7 +2045,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix) struct repository subrepo; if (argc != 2) - BUG("submodule--helper connect-gitdir-workingtree "); + BUG("submodule--helper ensure-core-worktree "); path = argv[1]; -- 2.20.0.rc2.403.gdbc3b29805-goog
Re: [PATCH] commit: abort before commit-msg if empty message
Hi, Jonathan Tan wrote: > (The implementation in this commit reads the commit message twice even > if there is no commit-msg hook. I think that this is fine, since this is > for interactive use - an alternative would be to plumb information about > the absence of the hook from run_hook_ve() upwards, which seems more > complicated.) Sounds like a good followup for an interested person to do later. Can you include a NEEDSWORK comment describing this? > Signed-off-by: Jonathan Tan > --- > This was noticed with the commit-msg hook that comes with Gerrit, which > basically just calls git interpret-trailers to add a Change-Id trailer. Thanks for fixing it. This kind of context tends to be very useful when looking back at a commit later, so I'd be happy to see it in the commit message too. [...] > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -652,6 +652,21 @@ static void adjust_comment_line_char(const struct strbuf > *sb) > comment_line_char = *p; > } > > +static void read_and_clean_commit_message(struct strbuf *sb) > +{ > + if (strbuf_read_file(sb, git_path_commit_editmsg(), 0) < 0) { > + int saved_errno = errno; > + rollback_index_files(); > + die(_("could not read commit message: %s"), > strerror(saved_errno)); Long line. More importantly, I wonder if this can use die_errno. rollback_index_files calls delete_tempfile, which can clobber errno, so that would require restoring errno either here or in that function: diff --git i/lockfile.h w/lockfile.h index 35403ccc0d..d592f384e7 100644 --- i/lockfile.h +++ w/lockfile.h @@ -298,10 +298,14 @@ static inline int commit_lock_file_to(struct lock_file *lk, const char *path) * remove the lockfile. It is a NOOP to call `rollback_lock_file()` * for a `lock_file` object that has already been committed or rolled * back. + * + * Saves and restores errno for more convenient use during error handling. */ static inline void rollback_lock_file(struct lock_file *lk) { + int save_errno = errno; delete_tempfile(>tempfile); + errno = save_errno; } #endif /* LOCKFILE_H */ It doesn't make the code more obvious so what you have is probably better. Also, on second glance, this is just moved code. Still hopefully some of the above is amusing (and rewrapping would be fine to do during the move). [...] > @@ -970,6 +985,22 @@ static int prepare_to_commit(const char *index_file, > const char *prefix, > argv_array_clear(); > } > > + if (use_editor && !no_verify) { > + /* > + * Abort the commit if the user supplied an empty commit > + * message in the editor. (Because the commit-msg hook is to be > + * run, we need to check this now, since that hook may change > + * the commit message.) > + */ > + read_and_clean_commit_message(); > + if (message_is_empty(, cleanup_mode) && > !allow_empty_message) { > + rollback_index_files(); > + fprintf(stderr, _("Aborting commit due to empty commit > message.\n")); > + exit(1); What about the template_untouched case? Should the two call sites share code? [...] > + } > + strbuf_release(); > + } > + > if (!no_verify && > run_commit_hook(use_editor, index_file, "commit-msg", > git_path_commit_editmsg(), NULL)) { > return 0; This means we have a little duplication of code: first we check whether to abort here in prepare_to_commit, and then again after the hook is run in cmd_commit. Is there some other code structure that would make things more clear? cmd_commit also seems to be rather long --- is there some logical way to split it up that would make the code clearer (as a preparatory or followup patch)? In cmd_commit, we spend a while (in number of lines, not actual running time) to determine parents before deciding whether the user has chosen to abort by writing an empty message. Should we perform that check sooner, closer to the prepare_to_commit call? > @@ -1608,17 +1639,7 @@ int cmd_commit(int argc, const char **argv, const char > *prefix) > > /* Finally, get the commit message */ > strbuf_reset(); > - if (strbuf_read_file(, git_path_commit_editmsg(), 0) < 0) { > - int saved_errno = errno; > - rollback_index_files(); > - die(_("could not read commit message: %s"), > strerror(saved_errno)); > - } > - > - if (verbose || /* Truncate the message just before the diff, if any. */ > - cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) > - strbuf_setlen(, wt_status_locate_end(sb.buf, sb.len)); > - if (cleanup_mode != COMMIT_MSG_CLEANUP_NONE) > - strbuf_stripspace(, cleanup_mode == COMMIT_MSG_CLEANUP_ALL); > + read_and_clean_commit_message(); > > if (message_is_empty(, cleanup_mode) &&
Re: [RFC PATCH 2/3] Documentation/git-rev-list: s///
On Sun, Oct 21, 2018 at 7:21 PM Junio C Hamano wrote: > > Matthew DeVore writes: > > >> It is more like "this is a set operation across commits. We also > >> show objects that are reachable from the commits in the resulting > >> set and are not reachable from the commits in the set that were > >> excluded when --objects option is given". > >> > > That would be correct though it wouldn't tell that you can use > > "--objects ^foo-tree bar-tree." > > Yeah, but quite honestly, I consider that it is working by accident, > not by design, in the current code (iow, you are looking at a > behaviour of whatever the code happens to do). "rev-list" is pretty > much set operation across commits, and anything that deals with a > non commit-ish given from the command line is an afterthought at > best, and happenstance in reality. > > I do not mean to say that the code must stay that way, though. I tried fixing the issue of "--objects ^commitobj treeobj" not properly excluding objects reachable from commitobj, but this ended up causing t5616-partial-clone.sh to fail. In the test labeled "manual prefetch of missing objects", we create a clone of srv.bare without blobs called "pc1", then push some new commits to srv.bare (via a separate "local" repo), and try to fetch missing blobs with this command: $ git -C pc1 fetch-pack --stdin "file://$(pwd)/srv.bare"
[PATCH] commit: abort before commit-msg if empty message
When a user runs "git commit" without specifying a message, an editor appears with advice: Please enter the commit message for your changes. Lines starting with '#' will be ignored, and an empty message aborts the commit. However, if the user supplies an empty message and has a commit-msg hook which updates the message to be non-empty, the commit proceeds to occur, despite what the advice states. Teach commit to also check the emptiness of the commit message before it invokes the commit-msg hook if an editor is used and if no_verify is not set (that is, commit-msg is not suppressed). This makes the advice true. (The implementation in this commit reads the commit message twice even if there is no commit-msg hook. I think that this is fine, since this is for interactive use - an alternative would be to plumb information about the absence of the hook from run_hook_ve() upwards, which seems more complicated.) Signed-off-by: Jonathan Tan --- This was noticed with the commit-msg hook that comes with Gerrit, which basically just calls git interpret-trailers to add a Change-Id trailer. --- builtin/commit.c | 43 -- t/t7504-commit-msg-hook.sh | 11 ++ 2 files changed, 43 insertions(+), 11 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index c021b119bb..3681a59af8 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -652,6 +652,21 @@ static void adjust_comment_line_char(const struct strbuf *sb) comment_line_char = *p; } +static void read_and_clean_commit_message(struct strbuf *sb) +{ + if (strbuf_read_file(sb, git_path_commit_editmsg(), 0) < 0) { + int saved_errno = errno; + rollback_index_files(); + die(_("could not read commit message: %s"), strerror(saved_errno)); + } + + if (verbose || /* Truncate the message just before the diff, if any. */ + cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) + strbuf_setlen(sb, wt_status_locate_end(sb->buf, sb->len)); + if (cleanup_mode != COMMIT_MSG_CLEANUP_NONE) + strbuf_stripspace(sb, cleanup_mode == COMMIT_MSG_CLEANUP_ALL); +} + static int prepare_to_commit(const char *index_file, const char *prefix, struct commit *current_head, struct wt_status *s, @@ -970,6 +985,22 @@ static int prepare_to_commit(const char *index_file, const char *prefix, argv_array_clear(); } + if (use_editor && !no_verify) { + /* +* Abort the commit if the user supplied an empty commit +* message in the editor. (Because the commit-msg hook is to be +* run, we need to check this now, since that hook may change +* the commit message.) +*/ + read_and_clean_commit_message(); + if (message_is_empty(, cleanup_mode) && !allow_empty_message) { + rollback_index_files(); + fprintf(stderr, _("Aborting commit due to empty commit message.\n")); + exit(1); + } + strbuf_release(); + } + if (!no_verify && run_commit_hook(use_editor, index_file, "commit-msg", git_path_commit_editmsg(), NULL)) { return 0; @@ -1608,17 +1639,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) /* Finally, get the commit message */ strbuf_reset(); - if (strbuf_read_file(, git_path_commit_editmsg(), 0) < 0) { - int saved_errno = errno; - rollback_index_files(); - die(_("could not read commit message: %s"), strerror(saved_errno)); - } - - if (verbose || /* Truncate the message just before the diff, if any. */ - cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) - strbuf_setlen(, wt_status_locate_end(sb.buf, sb.len)); - if (cleanup_mode != COMMIT_MSG_CLEANUP_NONE) - strbuf_stripspace(, cleanup_mode == COMMIT_MSG_CLEANUP_ALL); + read_and_clean_commit_message(); if (message_is_empty(, cleanup_mode) && !allow_empty_message) { rollback_index_files(); diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh index 31b9c6a2c1..b44d6fc43e 100755 --- a/t/t7504-commit-msg-hook.sh +++ b/t/t7504-commit-msg-hook.sh @@ -122,6 +122,17 @@ test_expect_success 'with failing hook (editor)' ' ' +test_expect_success 'hook is not run if commit message was empty' ' + echo "yet more another" >>file && + git add file && + echo >FAKE_MSG && + test_must_fail env GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit 2>err && + + # Verify that git stopped because it saw an empty message, not because + # the hook exited with non-zero error code + test_i18ngrep "Aborting commit due to empty commit message" err +' +
Re: enhancement: support for author.email and author.name in "git config"
William Hubbs wrote: > On Thu, Dec 06, 2018 at 11:20:07PM +0100, Johannes Schindelin wrote: >> There *is* a way to get what you want that is super easy and will >> definitely work: if you sit down and do it ;-) >> >> Please let us know if you need any additional information before you can >> start. > > Which branch should I work off of in the repo? "master". Also, please make sure the documentation (e.g. in Documentation/config/user.txt) describes when a user would want to set this option. See also: - Documentation/SubmittingPatches - the DISCUSSION section of "git help format-patch" - [1] - https://www.kernel.org/pub/software/scm/git/docs/user-manual.html#hacking-git Thanks, Jonathan [1] https://github.com/gitgitgadget/gitgitgadget#a-bot-to-serve-as-glue-between-github-pull-requests-and-the-git-mailing-list
Re: RFE: git-patch-id should handle patches without leading "diff"
Ævar Arnfjörð Bjarmason wrote: > On Fri, Dec 07 2018, Jonathan Nieder wrote: >> The patch-id appears to only care about the diff text, so it should be >> able to handle this. So if we have a better heuristic for where the >> diff starts, it would be good to use it. > > No, the patch-id doesn't just care about the diff, it cares about the > context before the diff too. Sorry, I did a bad job of communicating. When I said "diff text", I was including context. [...] > Observe that the diff --git line matters, we hash it: > > $ git diff-tree -p HEAD~.. | git patch-id > 5870d115b7e2a9a936ab8fdc254932234413c710 > > $ git diff-tree --src-prefix=a/ --dst-prefix=b/ -p HEAD~.. | git patch-id > --stable > 5870d115b7e2a9a936ab8fdc254932234413c710 > > $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~.. | git patch-id > --stable > 4cd136f2b98760150f700ac6a5b126389d6d05a7 > Oh, hm. That's unfortunate. [...] > So it seems most sensible to me if this is going to be supported that we > go a bit beyond the call of duty and fake up the start of it, namely: > > --- a/arch/x86/kernel/process.c > +++ b/arch/x86/kernel/process.c > > To be: > > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c > --- a/arch/x86/kernel/process.c > +++ b/arch/x86/kernel/process.c Right. We may want to handle diff.mnemonicPrefix as well. Jonathan
[PATCH v3 0/3] Add commit-graph fuzzer and fix buffer overflow
Ad a new fuzz test for the commit graph and fix a buffer read-overflow that it discovered. Additionally, fix the Makefile instructions for building fuzzers. Changes since V2: * Avoid pointer arithmetic overflow when checking the graph's chunk count. * Merge the corrupt_graph_and_verify and corrupt_and_zero_graph_then_verify test functions. Josh Steadmon (3): commit-graph, fuzz: Add fuzzer for commit-graph commit-graph: fix buffer read-overflow Makefile: correct example fuzz build .gitignore | 1 + Makefile| 3 +- commit-graph.c | 67 + commit-graph.h | 3 ++ fuzz-commit-graph.c | 16 ++ t/t5318-commit-graph.sh | 15 +++-- 6 files changed, 83 insertions(+), 22 deletions(-) create mode 100644 fuzz-commit-graph.c Range-diff against v2: 1: af45c2337f ! 1: 675d58ecea commit-graph: fix buffer read-overflow @@ -22,8 +22,8 @@ + uint64_t chunk_offset; int chunk_repeated = 0; -+ if (chunk_lookup + GRAPH_CHUNKLOOKUP_WIDTH > -+ data + graph_size) { ++ if (data + graph_size - chunk_lookup < ++ GRAPH_CHUNKLOOKUP_WIDTH) { + error(_("chunk lookup table entry missing; graph file may be incomplete")); + free(graph); + return NULL; @@ -40,31 +40,34 @@ --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ - test_i18ngrep "$grepstr" err - } + GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4)) + GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES)) -+ -+# usage: corrupt_and_zero_graph_then_verify -+# Manipulates the commit-graph file at by inserting the data, -+# then zeros the file starting at . Finally, runs -+# 'git commit-graph verify' and places the output in the file 'err'. Tests 'err' -+# for the given string. -+corrupt_and_zero_graph_then_verify() { -+ corrupt_pos=$1 -+ data="${2:-\0}" -+ zero_pos=$3 -+ grepstr=$4 +-# usage: corrupt_graph_and_verify ++# usage: corrupt_graph_and_verify[] + # Manipulates the commit-graph file at the position +-# by inserting the data, then runs 'git commit-graph verify' ++# by inserting the data, optionally zeroing the file ++# starting at , then runs 'git commit-graph verify' + # and places the output in the file 'err'. Test 'err' for + # the given string. + corrupt_graph_and_verify() { + pos=$1 + data="${2:-\0}" + grepstr=$3 + orig_size=$(stat --format=%s $objdir/info/commit-graph) -+ cd "$TRASH_DIRECTORY/full" && -+ test_when_finished mv commit-graph-backup $objdir/info/commit-graph && -+ cp $objdir/info/commit-graph commit-graph-backup && -+ printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$corrupt_pos" conv=notrunc && ++ zero_pos=${4:-${orig_size}} + cd "$TRASH_DIRECTORY/full" && + test_when_finished mv commit-graph-backup $objdir/info/commit-graph && + cp $objdir/info/commit-graph commit-graph-backup && + printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc && + truncate --size=$zero_pos $objdir/info/commit-graph && + truncate --size=$orig_size $objdir/info/commit-graph && -+ test_must_fail git commit-graph verify 2>test_err && -+ grep -v "^+" test_err >err && -+ test_i18ngrep "$grepstr" err -+} + test_must_fail git commit-graph verify 2>test_err && + grep -v "^+" test_err >err + test_i18ngrep "$grepstr" err + } + + test_expect_success 'detect bad signature' ' corrupt_graph_and_verify 0 "\0" \ @@ -73,9 +76,9 @@ "incorrect checksum" ' -+test_expect_success 'detect truncated graph' ' -+ corrupt_and_zero_graph_then_verify $GRAPH_BYTE_CHUNK_COUNT "\xff" \ -+ $GRAPH_CHUNK_LOOKUP_OFFSET "chunk lookup table entry missing" ++test_expect_success 'detect incorrect chunk count' ' ++ corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\xff" \ ++ "chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET +' + test_expect_success 'git fsck (checks commit-graph)' ' 2: 7519fc76df = 2: 06a32bfe8b Makefile: correct example fuzz build -- 2.20.0.rc2.12.g4c11c11dec
[PATCH v3 1/3] commit-graph, fuzz: Add fuzzer for commit-graph
Breaks load_commit_graph_one() into a new function, parse_commit_graph(). The latter function operates on arbitrary buffers, which makes it suitable as a fuzzing target. Since parse_commit_graph() is only called by load_commit_graph_one() (and the fuzzer described below), we omit error messages that would be duplicated by the caller. Adds fuzz-commit-graph.c, which provides a fuzzing entry point compatible with libFuzzer (and possibly other fuzzing engines). Signed-off-by: Josh Steadmon --- .gitignore | 1 + Makefile| 1 + commit-graph.c | 53 ++--- commit-graph.h | 3 +++ fuzz-commit-graph.c | 16 ++ 5 files changed, 57 insertions(+), 17 deletions(-) create mode 100644 fuzz-commit-graph.c diff --git a/.gitignore b/.gitignore index 0d77ea5894..8bcf153ed9 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ +/fuzz-commit-graph /fuzz_corpora /fuzz-pack-headers /fuzz-pack-idx diff --git a/Makefile b/Makefile index 1a44c811aa..6b72f37c29 100644 --- a/Makefile +++ b/Makefile @@ -684,6 +684,7 @@ SCRIPTS = $(SCRIPT_SH_INS) \ ETAGS_TARGET = TAGS +FUZZ_OBJS += fuzz-commit-graph.o FUZZ_OBJS += fuzz-pack-headers.o FUZZ_OBJS += fuzz-pack-idx.o diff --git a/commit-graph.c b/commit-graph.c index 40c855f185..07dd410f3c 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -84,16 +84,10 @@ static int commit_graph_compatible(struct repository *r) struct commit_graph *load_commit_graph_one(const char *graph_file) { void *graph_map; - const unsigned char *data, *chunk_lookup; size_t graph_size; struct stat st; - uint32_t i; - struct commit_graph *graph; + struct commit_graph *ret; int fd = git_open(graph_file); - uint64_t last_chunk_offset; - uint32_t last_chunk_id; - uint32_t graph_signature; - unsigned char graph_version, hash_version; if (fd < 0) return NULL; @@ -108,27 +102,55 @@ struct commit_graph *load_commit_graph_one(const char *graph_file) die(_("graph file %s is too small"), graph_file); } graph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0); + ret = parse_commit_graph(graph_map, fd, graph_size); + + if (!ret) { + munmap(graph_map, graph_size); + close(fd); + exit(1); + } + + return ret; +} + +struct commit_graph *parse_commit_graph(void *graph_map, int fd, + size_t graph_size) +{ + const unsigned char *data, *chunk_lookup; + uint32_t i; + struct commit_graph *graph; + uint64_t last_chunk_offset; + uint32_t last_chunk_id; + uint32_t graph_signature; + unsigned char graph_version, hash_version; + + if (!graph_map) + return NULL; + + if (graph_size < GRAPH_MIN_SIZE) + return NULL; + data = (const unsigned char *)graph_map; graph_signature = get_be32(data); if (graph_signature != GRAPH_SIGNATURE) { error(_("graph signature %X does not match signature %X"), graph_signature, GRAPH_SIGNATURE); - goto cleanup_fail; + return NULL; } graph_version = *(unsigned char*)(data + 4); if (graph_version != GRAPH_VERSION) { error(_("graph version %X does not match version %X"), graph_version, GRAPH_VERSION); - goto cleanup_fail; + return NULL; } hash_version = *(unsigned char*)(data + 5); if (hash_version != GRAPH_OID_VERSION) { error(_("hash version %X does not match version %X"), hash_version, GRAPH_OID_VERSION); - goto cleanup_fail; + return NULL; } graph = alloc_commit_graph(); @@ -152,7 +174,8 @@ struct commit_graph *load_commit_graph_one(const char *graph_file) if (chunk_offset > graph_size - GIT_MAX_RAWSZ) { error(_("improper chunk offset %08x%08x"), (uint32_t)(chunk_offset >> 32), (uint32_t)chunk_offset); - goto cleanup_fail; + free(graph); + return NULL; } switch (chunk_id) { @@ -187,7 +210,8 @@ struct commit_graph *load_commit_graph_one(const char *graph_file) if (chunk_repeated) { error(_("chunk id %08x appears multiple times"), chunk_id); - goto cleanup_fail; + free(graph); + return NULL; } if (last_chunk_id == GRAPH_CHUNKID_OIDLOOKUP) @@ -201,11 +225,6 @@ struct commit_graph *load_commit_graph_one(const char *graph_file) } return graph; - -cleanup_fail: -
[PATCH v3 2/3] commit-graph: fix buffer read-overflow
fuzz-commit-graph identified a case where Git will read past the end of a buffer containing a commit graph if the graph's header has an incorrect chunk count. A simple bounds check in parse_commit_graph() prevents this. Signed-off-by: Josh Steadmon --- commit-graph.c | 14 -- t/t5318-commit-graph.sh | 15 +-- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/commit-graph.c b/commit-graph.c index 07dd410f3c..836d65a1d3 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -165,10 +165,20 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd, last_chunk_offset = 8; chunk_lookup = data + 8; for (i = 0; i < graph->num_chunks; i++) { - uint32_t chunk_id = get_be32(chunk_lookup + 0); - uint64_t chunk_offset = get_be64(chunk_lookup + 4); + uint32_t chunk_id; + uint64_t chunk_offset; int chunk_repeated = 0; + if (data + graph_size - chunk_lookup < + GRAPH_CHUNKLOOKUP_WIDTH) { + error(_("chunk lookup table entry missing; graph file may be incomplete")); + free(graph); + return NULL; + } + + chunk_id = get_be32(chunk_lookup + 0); + chunk_offset = get_be64(chunk_lookup + 4); + chunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH; if (chunk_offset > graph_size - GIT_MAX_RAWSZ) { diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index 5fe21db99f..5b6b44b78e 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -366,24 +366,30 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4)) GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES)) -# usage: corrupt_graph_and_verify +# usage: corrupt_graph_and_verify[] # Manipulates the commit-graph file at the position -# by inserting the data, then runs 'git commit-graph verify' +# by inserting the data, optionally zeroing the file +# starting at , then runs 'git commit-graph verify' # and places the output in the file 'err'. Test 'err' for # the given string. corrupt_graph_and_verify() { pos=$1 data="${2:-\0}" grepstr=$3 + orig_size=$(stat --format=%s $objdir/info/commit-graph) + zero_pos=${4:-${orig_size}} cd "$TRASH_DIRECTORY/full" && test_when_finished mv commit-graph-backup $objdir/info/commit-graph && cp $objdir/info/commit-graph commit-graph-backup && printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc && + truncate --size=$zero_pos $objdir/info/commit-graph && + truncate --size=$orig_size $objdir/info/commit-graph && test_must_fail git commit-graph verify 2>test_err && grep -v "^+" test_err >err test_i18ngrep "$grepstr" err } + test_expect_success 'detect bad signature' ' corrupt_graph_and_verify 0 "\0" \ "graph signature" @@ -484,6 +490,11 @@ test_expect_success 'detect invalid checksum hash' ' "incorrect checksum" ' +test_expect_success 'detect incorrect chunk count' ' + corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\xff" \ + "chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET +' + test_expect_success 'git fsck (checks commit-graph)' ' cd "$TRASH_DIRECTORY/full" && git fsck && -- 2.20.0.rc2.12.g4c11c11dec
[PATCH v3 3/3] Makefile: correct example fuzz build
Signed-off-by: Josh Steadmon --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 6b72f37c29..bbcfc2bc9f 100644 --- a/Makefile +++ b/Makefile @@ -3104,7 +3104,7 @@ cover_db_html: cover_db # An example command to build against libFuzzer from LLVM 4.0.0: # # make CC=clang CXX=clang++ \ -# FUZZ_CXXFLAGS="-fsanitize-coverage=trace-pc-guard -fsanitize=address" \ +# CFLAGS="-fsanitize-coverage=trace-pc-guard -fsanitize=address" \ # LIB_FUZZING_ENGINE=/usr/lib/llvm-4.0/lib/libFuzzer.a \ # fuzz-all # -- 2.20.0.rc2.12.g4c11c11dec
Re: RFE: git-patch-id should handle patches without leading "diff"
On Fri, Dec 07 2018, Jonathan Nieder wrote: > Hi, > > Konstantin Ryabitsev wrote: > >> Every now and again I come across a patch sent to LKML without a leading >> "diff a/foo b/foo" -- usually produced by quilt. E.g.: >> >> https://lore.kernel.org/lkml/20181125185004.151077...@linutronix.de/ >> >> I am guessing quilt does not bother including the leading "diff a/foo >> b/foo" because it's redundant with the next two lines, however this >> remains a valid patch recognized by git-am. >> >> If you pipe that patch via git-patch-id, it produces nothing, but if I >> put in the leading "diff", like so: >> >> diff a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c >> >> then it properly returns "fb3ae17451bc619e3d7f0dd647dfba2b9ce8992e". > > Interesting. As Ævar mentioned, the relevant code is > > /* Ignore commit comments */ > if (!patchlen && !starts_with(line, "diff ")) > continue; > > which is trying to handle a case where a line that is special to the > parser appears before the diff begins. > > The patch-id appears to only care about the diff text, so it should be > able to handle this. So if we have a better heuristic for where the > diff starts, it would be good to use it. No, the patch-id doesn't just care about the diff, it cares about the context before the diff too. See this patch: $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~.. diff --git x/refs/files-backend.c y/refs/files-backend.c index 9183875dad..dd8abe9185 100644 --- x/refs/files-backend.c +++ y/refs/files-backend.c @@ -180,7 +180,8 @@ static void files_reflog_path(struct files_ref_store *refs, break; case REF_TYPE_OTHER_PSEUDOREF: case REF_TYPE_MAIN_PSEUDOREF: - return files_reflog_path_other_worktrees(refs, sb, refname); + files_reflog_path_other_worktrees(refs, sb, refname); + break; case REF_TYPE_NORMAL: strbuf_addf(sb, "%s/logs/%s", refs->gitcommondir, refname); break; Observe that the diff --git line matters, we hash it: $ git diff-tree -p HEAD~.. | git patch-id 5870d115b7e2a9a936ab8fdc254932234413c710 $ git diff-tree --src-prefix=a/ --dst-prefix=b/ -p HEAD~.. | git patch-id --stable 5870d115b7e2a9a936ab8fdc254932234413c710 $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~.. | git patch-id --stable 4cd136f2b98760150f700ac6a5b126389d6d05a7 The thing it doesn't care about is the "index" between the "diff" and patch: $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~.. | grep -v ^index | git patch-id --stable 4cd136f2b98760150f700ac6a5b126389d6d05a7 We also care about the +++ and --- lines: $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~.. | grep -v ^index | perl -pe 's/^(\+\+\+|---).*/$1/g' | git patch-id 56985c2c38cce6079de2690082e1770a8e81214c Then we normalize the @@ line, e.g.: $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~.. | grep -v ^index | git patch-id 4cd136f2b98760150f700ac6a5b126389d6d05a7 $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~.. | grep -v ^index | perl -pe 's/\d+/123/g' | git patch-id 4cd136f2b98760150f700ac6a5b126389d6d05a7 There's other caveats (see the code, e.g. "strip space") but to a first approximation a patch id is a hash of something that looks like this: diff --git x/refs/files-backend.c y/refs/files-backend.c --- x/refs/files-backend.c +++ y/refs/files-backend.c @@ -123,123 +123,123 @@ static void files_reflog_path(struct files_ref_store *refs, break; case REF_TYPE_OTHER_PSEUDOREF: case REF_TYPE_MAIN_PSEUDOREF: - return files_reflog_path_other_worktrees(refs, sb, refname); + files_reflog_path_other_worktrees(refs, sb, refname); + break; case REF_TYPE_NORMAL: strbuf_addf(sb, "%s/logs/%s", refs->gitcommondir, refname); break; Which means that accepting a patch like this as input would actually give you a different patch-id than if it had the proper header. So it seems most sensible to me if this is going to be supported that we go a bit beyond the call of duty and fake up the start of it, namely: --- a/arch/x86/kernel/process.c +++ b/arch/x86/kernel/process.c To be: diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c --- a/arch/x86/kernel/process.c +++ b/arch/x86/kernel/process.c It'll make the state machine a bit more complex, but IMO it would suck more if
Re: [PATCH] mailmap: update brandon williams's email address
Hi, Stefan Beller wrote: > What would be more of interest is why we'd be interested in this patch > as there is no commit/patch sent by Brandon with this email in gits history. I think there's an implicit assumption in this question that isn't spelled out. Do I understand correctly that you're saying the main purpose of .mailmap is to figure out whether two commits are by the same author? My own uses of .mailmap primarily have a different purpose: to find out the preferred contact address for the author of a given commit. Thanks, Jonathan
Re: enhancement: support for author.email and author.name in "git config"
Hi Johannes, On Thu, Dec 06, 2018 at 11:20:07PM +0100, Johannes Schindelin wrote: > Hi William, > >[...] > > There *is* a way to get what you want that is super easy and will > definitely work: if you sit down and do it ;-) > > Please let us know if you need any additional information before you can > start. Which branch should I work off of in the repo? William
Re: [PATCH v2 3/3] Refactor mingw_cygwin_offset_1st_component()
Hi Torsten, On Fri, 7 Dec 2018, tbo...@web.de wrote: > diff --git a/compat/mingw-cygwin.c b/compat/mingw-cygwin.c > index 5552c3ac20..c379a72775 100644 > --- a/compat/mingw-cygwin.c > +++ b/compat/mingw-cygwin.c > @@ -10,10 +10,8 @@ size_t mingw_cygwin_skip_dos_drive_prefix(char **path) > size_t mingw_cygwin_offset_1st_component(const char *path) > { > char *pos = (char *)path; > - > - /* unc paths */ This comment is still useful (and now even more correct), and should stay. > - if (!skip_dos_drive_prefix() && > - is_dir_sep(pos[0]) && is_dir_sep(pos[1])) { > + if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) { > + /* unc path */ > /* skip server name */ > pos = strpbrk(pos + 2, "\\/"); > if (!pos) > @@ -22,7 +20,8 @@ size_t mingw_cygwin_offset_1st_component(const char *path) > do { > pos++; > } while (*pos && !is_dir_sep(*pos)); > + } else { > + skip_dos_drive_prefix(); > } > - Why remove this empty line? It structures the code quite nicely. The rest looks correct to me, Johannes > return pos + is_dir_sep(*pos) - path; > } > -- > 2.19.0.271.gfe8321ec05 > >
Re: [PATCH] mailmap: update brandon williams's email address
On Fri, Dec 7, 2018 at 1:40 PM Jonathan Nieder wrote: > > Brandon Williams wrote: > > > Signed-off-by: Brandon Williams > > --- > > .mailmap | 1 + > > 1 file changed, 1 insertion(+) > > I can confirm that this is indeed the same person. What would be more of interest is why we'd be interested in this patch as there is no commit/patch sent by Brandon with this email in gits history. Is that so you get cc'd on your private address and can follow things you worked on without being subscribed to the mailing list? (I'd be interested to see the use case in the commit message;) Thanks, Stefan
Re: RFE: git-patch-id should handle patches without leading "diff"
Hi, Konstantin Ryabitsev wrote: > Every now and again I come across a patch sent to LKML without a leading > "diff a/foo b/foo" -- usually produced by quilt. E.g.: > > https://lore.kernel.org/lkml/20181125185004.151077...@linutronix.de/ > > I am guessing quilt does not bother including the leading "diff a/foo > b/foo" because it's redundant with the next two lines, however this > remains a valid patch recognized by git-am. > > If you pipe that patch via git-patch-id, it produces nothing, but if I > put in the leading "diff", like so: > > diff a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c > > then it properly returns "fb3ae17451bc619e3d7f0dd647dfba2b9ce8992e". Interesting. As Ævar mentioned, the relevant code is /* Ignore commit comments */ if (!patchlen && !starts_with(line, "diff ")) continue; which is trying to handle a case where a line that is special to the parser appears before the diff begins. The patch-id appears to only care about the diff text, so it should be able to handle this. So if we have a better heuristic for where the diff starts, it would be good to use it. "git apply" uses apply.c::find_header, which is more permissive. Maybe it would be possible to unify these somehow. (I haven't looked closely enough to tell how painful that would be.) Thanks and hope that helps, Jonathan
Re: [wishlist] git submodule update --reset-hard
On Thu, Dec 6, 2018 at 5:23 PM Yaroslav Halchenko wrote: > > There was a proposal to "re-attach HEAD" in the submodule, i.e. > > if the branch branch points at the same commit, we don't need > > a detached HEAD, but could go with the branch instead. > > if I got the idea right, if we are talking about any branch, it > would also non-deterministic since who knows what left over branch(es) > point to that commit. Not sure if I would have used that ;) I would think we'd rather want to have it deterministic, i.e. something like 1) record branch name of the submodule 2) update submodules HEAD to to superprojects gitlink 3) if recorded branch (1) matches the sha1 of detached HEAD, have HEAD point to the branch instead. You notice a small inefficiency here as we write HEAD twice, so it could be reworded as: 1) compare superprojects gitlink with the submodules branch 2a) if equal, set submodules HEAD to branch 2b) if unequal set HEAD to gitlink value, resulting in detached HEAD Note that this idea of reattaching reflects the idea (a) below. > > a) "stay on submodule branch (i.e. HEAD still points at $branch), and > > reset --hard" such that the submodule has a clean index and at that $branch > > or > > b) "stay on submodule branch (i.e. HEAD still points at $branch), but > > $branch is > >set to the gitlink from the superproject, and then a reset --hard > >will have the worktree set to it as well. > NB "gitlink" -- just now discovered the thing for me. Thought it would be > called a subproject echoing what git diff/log -p shows for submodule > commits. The terminology is messy: The internal representation in Gits object model is a "gitlink" entry in a tree object. Once we have a .gitmodules entry, we call it submodule. The term 'subproject' is a historic artifact and will likely not be changed in the diff output (or format-patch), because these diffs can be applied using git-am for example. That makes the diff output effectively a transport protocol, and changing protocols is hard if you have no versioning in them. More in https://git-scm.com/docs/gitsubmodules (a rather recent new write of a man page, going into concepts). > > > right -- I meant the local changes and indeed reset --recurse-submodules > > > indeed seems to recurse nicely. Then the undesired effect remaining only > > > the detached HEAD > > > For that we may want to revive discussions in > > https://public-inbox.org/git/20170501180058.8063-5-sbel...@google.com/ > > well, isn't that one requires a branch to be specified in .gitmodules? Ah good point. > > git reset --hard --recursive=hard,keep-branch PREVIOUSPOINT > > 'keep-branch' (given aforementioned keeping the specified in .gitmodules > branch) might be confusing. Also what if a submodule already in a > detached HEAD? IMHO --recursive=hard, and just saying that it would do > "reset --hard", is imho sufficient. (that is why I like pure > --reset hard since it doesn't care and neither does anything to the > branch) For that we might want to first do the git submodule update --reset-hard which runs reset --hard inside the submodule, no matter which branch the submodule is on (if any) and resets to the given superproject sha1. See git-submodule.sh in git.git[1] in cmd_update. We'd need to add a command line flag (`--reset-hard` would be the obvious choice?) which would set the `update` variable, which then is evaluated to what needs to be done in the submodule, which in that case would be the hard reset. https://github.com/git/git/blob/master/git-submodule.sh#L606 Once that is done we'd want to add a test case, presumably in t/t7406-submodule-update.sh > > > I would have asked for > > > >git revert --recursive ... > > >git rebase --recursive [-i] ... > > > > which I also frequently desire (could elaborate on the use cases etc). > > > These would be nice to have. It would be nice if you'd elaborate on the > > use cases for future reference in the mailing list archive. :-) > > ok, will try to do so ;-) In summary: they are just a logical extension > of git support for submodules for anyone actively working with > submodules to keep entire tree in sync. Then quite often the need for > reverting a specific commit (which also has changes reflected in > submodules) arises. The same with rebase, especially to trim away some > no longer desired changes reflected in submodules. > > the initial "git submodule update --reset-hard" is pretty much a > crude workaround for some of those cases, so I would just go earlier in > the history, and redo some things, whenever I could just drop or revert > some selected set of commits. That makes sense. Do you want to give the implementation a try for the --reset-hard switch? > ah... so it is only submodule command which has --recursive, and the > rest have --recurse-submodules when talking about recursing into > submodules? I don't think we were that cautious in development as it was done by different
Re: [PATCH v2 1/3] git clone C:\cygwin\home\USER\repo' is working (again)
Hi Torsten, On Fri, 7 Dec 2018, tbo...@web.de wrote: > compat/mingw-cygwin.c | 28 > compat/mingw-cygwin.h | 20 Please use compat/win32/path.c (or .../path-utils.c) instead. This is not so much about MINGW or Cygwin or MSys or MSYS2 or Visual C++, but about Windows. Thanks, Johannes
Re: Retrieving a file in git that was deleted and committed
On Thu, Dec 6, 2018 at 11:26 PM Jeff King wrote: >> >> On Thu, Dec 06, 2018 at 11:07:00PM -0800, biswaranjan panda wrote: >> > >> Thanks! Strangely git log --follow does work. >> >> I suspect it would work even without --follow. When you limit a log >> traversal with a pathspec, like: >> >> git log foo >> >> that is not about following some continuous stream of content, but >> rather just applying that pathspec to the diff of each commit, and >> pruning ones where it did not change. So even if there are gaps where >> the file did not exist, we continue to apply the pathspec to the older >> commits. > Ah, of course. Thanks for the clarification, Jeff. And my > apologies to > Biswaranjan Panda for the incorrect information. Thanks Jeff and Bryan! However, I am curious that if there were a way to tell git blame to skip a commit (the one which added the file again and maybe the one which deleted it originally) while it walks back through history, then it should just get back the entire history right ? On Thu, Dec 6, 2018 at 11:37 PM Bryan Turner wrote: > > On Thu, Dec 6, 2018 at 11:26 PM Jeff King wrote: > > > > On Thu, Dec 06, 2018 at 11:07:00PM -0800, biswaranjan panda wrote: > > > > > Thanks! Strangely git log --follow does work. > > > > I suspect it would work even without --follow. When you limit a log > > traversal with a pathspec, like: > > > > git log foo > > > > that is not about following some continuous stream of content, but > > rather just applying that pathspec to the diff of each commit, and > > pruning ones where it did not change. So even if there are gaps where > > the file did not exist, we continue to apply the pathspec to the older > > commits. > > Ah, of course. Thanks for the clarification, Jeff. And my apologies to > Biswaranjan Panda for the incorrect information. > > > > > Tools like git-blame will _not_ work, though, as they really are trying > > to track the content as they walk back through history. And Once all of > > the content seems to appear from nowhere in your new commit, that seems > > like a dead end. > > > > In theory there could be some machine-readable annotation in the commit > > object (or in a note created after the fact) to say "even though 'foo' > > is a new file here, it came from $commit:foo". And then git-blame could > > keep following the content there. But such a feature does not yet exist. > > > > -Peff -- Thanks, -Biswa
[PATCH on master v2] revision: use commit graph in get_reference()
When fetching into a repository, a connectivity check is first made by check_exist_and_connected() in builtin/fetch.c that runs: git rev-list --objects --stdin --not --all --quiet <(list of objects) If the client repository has many refs, this command can be slow, regardless of the nature of the server repository or what is being fetched. A profiler reveals that most of the time is spent in setup_revisions() (approx. 60/63), and of the time spent in setup_revisions(), most of it is spent in parse_object() (approx. 49/60). This is because setup_revisions() parses the target of every ref (from "--all"), and parse_object() reads the buffer of the object. Reading the buffer is unnecessary if the repository has a commit graph and if the ref points to a commit (which is typically the case). This patch uses the commit graph wherever possible; on my computer, when I run the above command with a list of 1 object on a many-ref repository, I get a speedup from 1.8s to 1.0s. Another way to accomplish this effect would be to modify parse_object() to use the commit graph if possible; however, I did not want to change parse_object()'s current behavior of always checking the object signature of the returned object. Signed-off-by: Jonathan Tan --- This patch is now on master. v2 makes use of the optimization Stolee describes in [1], except that I have arranged the functions slightly differently. In particular, I didn't want to add even more ways to obtain objects, so I let parse_commit_in_graph() be able to take in either a commit shell or an OID, and did not create the parse_probably_commit() function he suggested. But I'm not really attached to this design choice, and can change it if requested. [1] https://public-inbox.org/git/aa0cd481-c135-47aa-2a69-e3dc71661...@gmail.com/ --- commit-graph.c | 38 -- commit-graph.h | 12 commit.c | 2 +- revision.c | 5 - t/helper/test-repository.c | 4 ++-- 5 files changed, 43 insertions(+), 18 deletions(-) diff --git a/commit-graph.c b/commit-graph.c index 40c855f185..a571b523b7 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -286,7 +286,8 @@ void close_commit_graph(struct repository *r) r->objects->commit_graph = NULL; } -static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos) +static int bsearch_graph(struct commit_graph *g, const struct object_id *oid, +uint32_t *pos) { return bsearch_hash(oid->hash, g->chunk_oid_fanout, g->chunk_oid_lookup, g->hash_len, pos); @@ -374,24 +375,41 @@ static int find_commit_in_graph(struct commit *item, struct commit_graph *g, uin } } -static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item) +static struct commit *parse_commit_in_graph_one(struct repository *r, + struct commit_graph *g, + struct commit *shell, + const struct object_id *oid) { uint32_t pos; - if (item->object.parsed) - return 1; + if (shell && shell->object.parsed) + return shell; - if (find_commit_in_graph(item, g, )) - return fill_commit_in_graph(item, g, pos); + if (shell && shell->graph_pos != COMMIT_NOT_FROM_GRAPH) { + pos = shell->graph_pos; + } else if (bsearch_graph(g, shell ? >object.oid : oid, )) { + /* bsearch_graph sets pos */ + } else { + return NULL; + } - return 0; + if (!shell) { + shell = lookup_commit(r, oid); + if (!shell) + return NULL; + } + + fill_commit_in_graph(shell, g, pos); + return shell; } -int parse_commit_in_graph(struct repository *r, struct commit *item) +struct commit *parse_commit_in_graph(struct repository *r, struct commit *shell, +const struct object_id *oid) { if (!prepare_commit_graph(r)) return 0; - return parse_commit_in_graph_one(r->objects->commit_graph, item); + return parse_commit_in_graph_one(r, r->objects->commit_graph, shell, +oid); } void load_commit_graph_info(struct repository *r, struct commit *item) @@ -1025,7 +1043,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g) } graph_commit = lookup_commit(r, _oid); - if (!parse_commit_in_graph_one(g, graph_commit)) + if (!parse_commit_in_graph_one(r, g, graph_commit, NULL)) graph_report("failed to parse %s from commit-graph", oid_to_hex(_oid)); } diff --git a/commit-graph.h b/commit-graph.h index
Re: [PATCH] mailmap: update brandon williams's email address
Brandon Williams wrote: > Signed-off-by: Brandon Williams > --- > .mailmap | 1 + > 1 file changed, 1 insertion(+) I can confirm that this is indeed the same person. Reviewed-by: Jonathan Nieder Welcome back!
[PATCH] mailmap: update brandon williams's email address
Signed-off-by: Brandon Williams --- .mailmap | 1 + 1 file changed, 1 insertion(+) diff --git a/.mailmap b/.mailmap index eb7b5fc7b..247a3deb7 100644 --- a/.mailmap +++ b/.mailmap @@ -27,6 +27,7 @@ Ben Walton Benoit Sigoure Bernt Hansen Brandon Casey +Brandon Williams brian m. carlson brian m. carlson Bryan Larsen -- 2.19.1
Re: RFE: git-patch-id should handle patches without leading "diff"
On Fri, Dec 07 2018, Konstantin Ryabitsev wrote: > Hi, all: > > Every now and again I come across a patch sent to LKML without a leading > "diff a/foo b/foo" -- usually produced by quilt. E.g.: > > https://lore.kernel.org/lkml/20181125185004.151077...@linutronix.de/ > > I am guessing quilt does not bother including the leading "diff a/foo > b/foo" because it's redundant with the next two lines, however this > remains a valid patch recognized by git-am. > > If you pipe that patch via git-patch-id, it produces nothing, but if I > put in the leading "diff", like so: > > diff a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c > > then it properly returns "fb3ae17451bc619e3d7f0dd647dfba2b9ce8992e". > > Can we please teach git-patch-id to work without the leading diff a/foo > b/foo, same as git-am? > > Best, > -K The state machine is sensitive there being a "diff" line, then "index" etc. diff --git a/builtin/patch-id.c b/builtin/patch-id.c index 970d0d30b4..b99e4455fd 100644 --- a/builtin/patch-id.c +++ b/builtin/patch-id.c @@ -97,7 +97,9 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result, } /* Ignore commit comments */ - if (!patchlen && !starts_with(line, "diff ")) + if (!patchlen && starts_with(line, "--- a/")) + ; + else if (!patchlen && !starts_with(line, "diff ")) continue; /* Parsing diff header? */ This would make it produce a patch-id for that input, however note that I've done "--- a/" there, with just "--- " (which is legit) we'd get confused and start earlier before the diffstat. So if you're interested in having this I leave it to you to run with this & write tests for it, but more convincingly run it on the git & LKML archives and see that the output is the same (or just extra in case where we now find patches) with --stable etc.
[PATCH] l10n: de.po: fix two messages
Reported-by: Phillip Szelat Signed-off-by: Ralf Thielow --- Hi Phillip, Good catches. Thanks for the review! Ralf po/de.po | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/po/de.po b/po/de.po index eb213d742..d5113434a 100644 --- a/po/de.po +++ b/po/de.po @@ -3421,7 +3421,7 @@ msgstr "Fehler bei Vorbereitung der Packdatei aus multi-pack-index." #: midx.c:407 #, c-format msgid "failed to add packfile '%s'" -msgstr "Fehler beim Hinzufügen von Packdatei'%s'." +msgstr "Fehler beim Hinzufügen von Packdatei '%s'." #: midx.c:413 #, c-format @@ -4559,7 +4559,7 @@ msgstr "Öffnen von /dev/null fehlgeschlagen" #: run-command.c:1229 #, c-format msgid "cannot create async thread: %s" -msgstr "Kann Thread für async nicht erzeugen: %s" +msgstr "Konnte Thread für async nicht erzeugen: %s" #: run-command.c:1293 #, c-format -- 2.20.0.rc2.411.g8f28e744c2
Re: [PATCH] git-rebase.txt: update note about directory rename detection and am
On Fri, Dec 7, 2018 at 9:51 AM Johannes Sixt wrote: > > From: Elijah Newren > > In commit 6aba117d5cf7 ("am: avoid directory rename detection when > calling recursive merge machinery", 2018-08-29), the git-rebase manpage > probably should have also been updated to note the stronger > incompatibility between git-am and directory rename detection. Update > it now. > > Signed-off-by: Elijah Newren > Signed-off-by: Johannes Sixt > --- > Documentation/git-rebase.txt | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index 41631df6e4..7bea21e8e3 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -569,8 +569,9 @@ it to keep commits that started empty. > Directory rename detection > ~~ > > -The merge and interactive backends work fine with > -directory rename detection. The am backend sometimes does not. > +Directory rename heuristics are enabled in the merge and interactive > +backends. Due to the lack of accurate tree information, directory > +rename detection is disabled in the am backend. > > include::merge-strategies.txt[] I was intending to send this out the past couple days, was just kinda busy. Thanks for handling it for me.
RFE: git-patch-id should handle patches without leading "diff"
Hi, all: Every now and again I come across a patch sent to LKML without a leading "diff a/foo b/foo" -- usually produced by quilt. E.g.: https://lore.kernel.org/lkml/20181125185004.151077...@linutronix.de/ I am guessing quilt does not bother including the leading "diff a/foo b/foo" because it's redundant with the next two lines, however this remains a valid patch recognized by git-am. If you pipe that patch via git-patch-id, it produces nothing, but if I put in the leading "diff", like so: diff a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c then it properly returns "fb3ae17451bc619e3d7f0dd647dfba2b9ce8992e". Can we please teach git-patch-id to work without the leading diff a/foo b/foo, same as git-am? Best, -K signature.asc Description: PGP signature
[PATCH] git-rebase.txt: update note about directory rename detection and am
From: Elijah Newren In commit 6aba117d5cf7 ("am: avoid directory rename detection when calling recursive merge machinery", 2018-08-29), the git-rebase manpage probably should have also been updated to note the stronger incompatibility between git-am and directory rename detection. Update it now. Signed-off-by: Elijah Newren Signed-off-by: Johannes Sixt --- Documentation/git-rebase.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index 41631df6e4..7bea21e8e3 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -569,8 +569,9 @@ it to keep commits that started empty. Directory rename detection ~~ -The merge and interactive backends work fine with -directory rename detection. The am backend sometimes does not. +Directory rename heuristics are enabled in the merge and interactive +backends. Due to the lack of accurate tree information, directory +rename detection is disabled in the am backend. include::merge-strategies.txt[] -- 2.19.1.1133.g2dd3d172d2
[PATCH v2 1/3] git clone C:\cygwin\home\USER\repo' is working (again)
From: Torsten Bögershausen A regression for cygwin users was introduced with commit 05b458c, "real_path: resolve symlinks by hand". In the the commit message we read: The current implementation of real_path uses chdir() in order to resolve symlinks. Unfortunately this isn't thread-safe as chdir() affects a process as a whole... The old (and non-thread-save) OS calls chdir()/pwd() had been replaced by a string operation. The cygwin layer "knows" that "C:\cygwin" is an absolute path, but the new string operation does not. "git clone C:\cygwin\home\USER\repo" fails like this: fatal: Invalid path '/home/USER/repo/C:\cygwin\home\USER\repo' The solution is to implement has_dos_drive_prefix(), skip_dos_drive_prefix() is_dir_sep(), offset_1st_component() and convert_slashes() for cygwin in the same way as it is done in 'Git for Windows' in compat/mingw.[ch] Instead of duplicating the code, it is extracted into compat/mingw-cygwin.[ch] Some need for refactoring and cleanup came up in the review, they are adressed in a seperate commit. Reported-By: Steven Penny Signed-off-by: Torsten Bögershausen --- compat/cygwin.c | 19 --- compat/cygwin.h | 2 -- compat/mingw-cygwin.c | 28 compat/mingw-cygwin.h | 20 compat/mingw.c| 29 + compat/mingw.h| 20 config.mak.uname | 4 ++-- git-compat-util.h | 3 ++- 8 files changed, 53 insertions(+), 72 deletions(-) delete mode 100644 compat/cygwin.c delete mode 100644 compat/cygwin.h create mode 100644 compat/mingw-cygwin.c create mode 100644 compat/mingw-cygwin.h diff --git a/compat/cygwin.c b/compat/cygwin.c deleted file mode 100644 index b9862d606d..00 --- a/compat/cygwin.c +++ /dev/null @@ -1,19 +0,0 @@ -#include "../git-compat-util.h" -#include "../cache.h" - -int cygwin_offset_1st_component(const char *path) -{ - const char *pos = path; - /* unc paths */ - if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) { - /* skip server name */ - pos = strchr(pos + 2, '/'); - if (!pos) - return 0; /* Error: malformed unc path */ - - do { - pos++; - } while (*pos && pos[0] != '/'); - } - return pos + is_dir_sep(*pos) - path; -} diff --git a/compat/cygwin.h b/compat/cygwin.h deleted file mode 100644 index 8e52de4644..00 --- a/compat/cygwin.h +++ /dev/null @@ -1,2 +0,0 @@ -int cygwin_offset_1st_component(const char *path); -#define offset_1st_component cygwin_offset_1st_component diff --git a/compat/mingw-cygwin.c b/compat/mingw-cygwin.c new file mode 100644 index 00..c63d7acb9c --- /dev/null +++ b/compat/mingw-cygwin.c @@ -0,0 +1,28 @@ +#include "../git-compat-util.h" + +int mingw_cygwin_skip_dos_drive_prefix(char **path) +{ + int ret = has_dos_drive_prefix(*path); + *path += ret; + return ret; +} + +int mingw_cygwin_offset_1st_component(const char *path) +{ + char *pos = (char *)path; + + /* unc paths */ + if (!skip_dos_drive_prefix() && + is_dir_sep(pos[0]) && is_dir_sep(pos[1])) { + /* skip server name */ + pos = strpbrk(pos + 2, "\\/"); + if (!pos) + return 0; /* Error: malformed unc path */ + + do { + pos++; + } while (*pos && !is_dir_sep(*pos)); + } + + return pos + is_dir_sep(*pos) - path; +} diff --git a/compat/mingw-cygwin.h b/compat/mingw-cygwin.h new file mode 100644 index 00..66ccc909ae --- /dev/null +++ b/compat/mingw-cygwin.h @@ -0,0 +1,20 @@ +#define has_dos_drive_prefix(path) \ + (isalpha(*(path)) && (path)[1] == ':' ? 2 : 0) +int mingw_cygwin_skip_dos_drive_prefix(char **path); +#define skip_dos_drive_prefix mingw_cygwin_skip_dos_drive_prefix +static inline int mingw_cygwin_is_dir_sep(int c) +{ + return c == '/' || c == '\\'; +} +#define is_dir_sep mingw_cygwin_is_dir_sep +static inline char *mingw_cygwin_find_last_dir_sep(const char *path) +{ + char *ret = NULL; + for (; *path; ++path) + if (is_dir_sep(*path)) + ret = (char *)path; + return ret; +} +#define find_last_dir_sep mingw_cygwin_find_last_dir_sep +int mingw_cygwin_offset_1st_component(const char *path); +#define offset_1st_component mingw_cygwin_offset_1st_component diff --git a/compat/mingw.c b/compat/mingw.c index 34b3880b29..038e96af9d 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -350,7 +350,7 @@ static inline int needs_hiding(const char *path) return 0; /* We cannot use basename(), as it would remove trailing slashes */ - mingw_skip_dos_drive_prefix((char **)); + mingw_cygwin_skip_dos_drive_prefix((char **)); if (!*path) return 0; @@ -2275,33
[PATCH v2 3/3] Refactor mingw_cygwin_offset_1st_component()
From: Torsten Bögershausen The Windows version of offset_1st_component() needs to hande 3 cases: - The path is an UNC path, starting with "//" or "". Skip the servername and the name of the share. - The path is a DOS drive, starting with e.g. "X:" The driver letter and the ':' must be skipped - The path is pointing to a subdirectory somewhere in the path and the directory seperator needs to be skipped ('/' or '\\'). Refactor the code to make it easier to read. Suggested-by: Johannes Schindelin Signed-off-by: Torsten Bögershausen --- compat/mingw-cygwin.c | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/compat/mingw-cygwin.c b/compat/mingw-cygwin.c index 5552c3ac20..c379a72775 100644 --- a/compat/mingw-cygwin.c +++ b/compat/mingw-cygwin.c @@ -10,10 +10,8 @@ size_t mingw_cygwin_skip_dos_drive_prefix(char **path) size_t mingw_cygwin_offset_1st_component(const char *path) { char *pos = (char *)path; - - /* unc paths */ - if (!skip_dos_drive_prefix() && - is_dir_sep(pos[0]) && is_dir_sep(pos[1])) { + if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) { + /* unc path */ /* skip server name */ pos = strpbrk(pos + 2, "\\/"); if (!pos) @@ -22,7 +20,8 @@ size_t mingw_cygwin_offset_1st_component(const char *path) do { pos++; } while (*pos && !is_dir_sep(*pos)); + } else { + skip_dos_drive_prefix(); } - return pos + is_dir_sep(*pos) - path; } -- 2.19.0.271.gfe8321ec05
[PATCH v2 2/3] offset_1st_component(), dos_drive_prefix() return size_t
From: Torsten Bögershausen Change the return value for offset_1st_component(), has_dos_drive_prefix() and skip_dos_drive_prefix from int into size_t, which is the natural type for length of data in memory. While at it, remove possible "parameter not used" warnings in for the non-Windows builds in git-compat-util.h Signed-off-by: Torsten Bögershausen --- abspath.c | 2 +- compat/mingw-cygwin.c | 6 +++--- compat/mingw-cygwin.h | 4 ++-- git-compat-util.h | 8 +--- setup.c | 4 ++-- 5 files changed, 13 insertions(+), 11 deletions(-) diff --git a/abspath.c b/abspath.c index 9857985329..12055a1d8f 100644 --- a/abspath.c +++ b/abspath.c @@ -51,7 +51,7 @@ static void get_next_component(struct strbuf *next, struct strbuf *remaining) /* copies root part from remaining to resolved, canonicalizing it on the way */ static void get_root_part(struct strbuf *resolved, struct strbuf *remaining) { - int offset = offset_1st_component(remaining->buf); + size_t offset = offset_1st_component(remaining->buf); strbuf_reset(resolved); strbuf_add(resolved, remaining->buf, offset); diff --git a/compat/mingw-cygwin.c b/compat/mingw-cygwin.c index c63d7acb9c..5552c3ac20 100644 --- a/compat/mingw-cygwin.c +++ b/compat/mingw-cygwin.c @@ -1,13 +1,13 @@ #include "../git-compat-util.h" -int mingw_cygwin_skip_dos_drive_prefix(char **path) +size_t mingw_cygwin_skip_dos_drive_prefix(char **path) { - int ret = has_dos_drive_prefix(*path); + size_t ret = has_dos_drive_prefix(*path); *path += ret; return ret; } -int mingw_cygwin_offset_1st_component(const char *path) +size_t mingw_cygwin_offset_1st_component(const char *path) { char *pos = (char *)path; diff --git a/compat/mingw-cygwin.h b/compat/mingw-cygwin.h index 66ccc909ae..0e8a0c9074 100644 --- a/compat/mingw-cygwin.h +++ b/compat/mingw-cygwin.h @@ -1,6 +1,6 @@ #define has_dos_drive_prefix(path) \ (isalpha(*(path)) && (path)[1] == ':' ? 2 : 0) -int mingw_cygwin_skip_dos_drive_prefix(char **path); +size_t mingw_cygwin_skip_dos_drive_prefix(char **path); #define skip_dos_drive_prefix mingw_cygwin_skip_dos_drive_prefix static inline int mingw_cygwin_is_dir_sep(int c) { @@ -16,5 +16,5 @@ static inline char *mingw_cygwin_find_last_dir_sep(const char *path) return ret; } #define find_last_dir_sep mingw_cygwin_find_last_dir_sep -int mingw_cygwin_offset_1st_component(const char *path); +size_t mingw_cygwin_offset_1st_component(const char *path); #define offset_1st_component mingw_cygwin_offset_1st_component diff --git a/git-compat-util.h b/git-compat-util.h index 7ece969b22..65eaaf0d50 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -355,16 +355,18 @@ static inline int noop_core_config(const char *var, const char *value, void *cb) #endif #ifndef has_dos_drive_prefix -static inline int git_has_dos_drive_prefix(const char *path) +static inline size_t git_has_dos_drive_prefix(const char *path) { + (void)path; return 0; } #define has_dos_drive_prefix git_has_dos_drive_prefix #endif #ifndef skip_dos_drive_prefix -static inline int git_skip_dos_drive_prefix(char **path) +static inline size_t git_skip_dos_drive_prefix(char **path) { + (void)path; return 0; } #define skip_dos_drive_prefix git_skip_dos_drive_prefix @@ -379,7 +381,7 @@ static inline int git_is_dir_sep(int c) #endif #ifndef offset_1st_component -static inline int git_offset_1st_component(const char *path) +static inline size_t git_offset_1st_component(const char *path) { return is_dir_sep(path[0]); } diff --git a/setup.c b/setup.c index 1be5037f12..538bc1ff99 100644 --- a/setup.c +++ b/setup.c @@ -29,7 +29,7 @@ static int abspath_part_inside_repo(char *path) size_t len; size_t wtlen; char *path0; - int off; + size_t off; const char *work_tree = get_git_work_tree(); if (!work_tree) @@ -800,7 +800,7 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset, struct repository_format *repo_fmt, int *nongit_ok) { - int root_len; + size_t root_len; if (check_repository_format_gently(".", repo_fmt, nongit_ok)) return NULL; -- 2.19.0.271.gfe8321ec05
Re: [PATCH on sb/more-repo-in-api] revision: use commit graph in get_reference()
On 12/6/2018 6:36 PM, Jonathan Tan wrote: AFAICT oid_object_info doesn't take advantage of the commit graph, but just looks up the object header, which is still less than completely parsing it. Then lookup_commit is overly strict, as it may return NULL as when there still is a type mismatch (I don't think a mismatch could happen here, as both rely on just the object store, and not the commit graph.), so this would be just defensive programming for the sake of it. I dunno. struct commit *c; if (oid_object_info(revs->repo, oid, NULL) == OBJ_COMMIT && (c = lookup_commit(revs->repo, oid)) && !repo_parse_commit(revs->repo, c)) object = (struct object *) c; else object = parse_object(revs->repo, oid); I like this way better - I'll do it in the next version. If we do _not_ have a commit-graph or if the commit-graph does not have that commit, this will have the same performance problem, right? Should we instead create a direct dependence on the commit-graph, and try to parse the oid from the graph directly? If it succeeds, then we learn that the object is a commit, in addition to all of the parsing work. This means we could avoid oid_object_info() loading data if we succeed. We would fall back to parse_object() if it fails. I was thinking this should be a simple API call to parse_commit_in_graph(), but that requires a struct commit filled with an oid, which is not the best idea if we don't actually know it is a commit yet. The approach I recommend would then be more detailed: 1. Modify find_commit_in_graph() to take a struct object_id instead of a struct commit. This helps find the integer position in the graph. That position can be used in fill_commit_in_graph() to load the commit contents. Keep find_commit_in_graph() static as it should not be a public function. 2. Create a public function with prototype struct commit *try_parse_commit_from_graph(struct repository *r, struct object_id *oid) that returns a commit struct fully parsed if and only if the repository has that oid. It can call find_commit_in_graph(), then lookup_commit() and fill_commit_in_graph() to create the commit and parse the data. 3. In replace of the snippet above, do: struct commit *c; if ((c = try_parse_commit_from_graph(revs->repo, oid)) object = (struct object *)c; else object = parse_object(revs->repo, oid); A similar pattern _could_ be used in parse_object(), but I don't recommend doing this pattern unless we have a reasonable suspicion that we are going to parse commits more often than other objects. (It adds an O(log(# commits)) binary search to each object.) A final thought: consider making this "try the commit graph first, but fall back to parse_object()" a library function with a name like struct object *parse_probably_commit(struct repository *r, struct object_id *oid) so other paths that are parsing a lot of commits (but also maybe tags) could use the logic. Thanks! -Stolee
Re: [PATCH v2 2/3] commit-graph: fix buffer read-overflow
On 12/6/2018 3:20 PM, Josh Steadmon wrote: + +# usage: corrupt_and_zero_graph_then_verify +# Manipulates the commit-graph file at by inserting the data, +# then zeros the file starting at . Finally, runs +# 'git commit-graph verify' and places the output in the file 'err'. Tests 'err' +# for the given string. +corrupt_and_zero_graph_then_verify() { This method is very similar to to 'corrupt_graph_and_verify()', the only difference being the zero_pos, which zeroes the graph. Could it instead be a modification of corrupt_graph_and_verify() where $4 is interpreted as zero_pos, and if it is blank we don't do the truncation? +test_expect_success 'detect truncated graph' ' + corrupt_and_zero_graph_then_verify $GRAPH_BYTE_CHUNK_COUNT "\xff" \ + $GRAPH_CHUNK_LOOKUP_OFFSET "chunk lookup table entry missing" +' + Thanks for this! I think it's valuable to keep explicit tests around that were discovered from your fuzz tests. Specifically, I can repeat the test when I get around to the next file format. Thanks, -Stolee
Re: Get "responsible" .gitignore file / rule
Am Fr., 7. Dez. 2018 um 13:45 Uhr schrieb Eric Sunshine : > > On Fri, Dec 7, 2018 at 7:36 AM Victor Toni wrote: > > I'm wondering if there is any way to show which rules (ideally with > > the .gitignore file they are coming from) are causing a specific file > > to get ignored so I could easily fix the .gitignore file? > > Perhaps the "git check-ignore" command would help. Thanks for the tip! Works like a charm (had to use the --verbose option though, without, it does not give much feedback)
Re: Get "responsible" .gitignore file / rule
On Fri, Dec 7, 2018 at 7:36 AM Victor Toni wrote: > I'm wondering if there is any way to show which rules (ideally with > the .gitignore file they are coming from) are causing a specific file > to get ignored so I could easily fix the .gitignore file? Perhaps the "git check-ignore" command would help.
Get "responsible" .gitignore file / rule
In a rather complex setup with deep directory structure it happens every now and then, that files get ignored when trying to add them. As these files are _not_ shown in `git status` but in `git status --ignored` so I guess the culprit is some misconfigured `.gitignore`. Trying to ad the specific file gives a: $ git add ignored/file/name The following paths are ignored by one of your .gitignore files: ignored/file/name Use -f if you really want to add them. Using -v doen't add any verbosity. I'm using git 2.19.1.windows.1 if this matters I'm wondering if there is any way to show which rules (ideally with the .gitignore file they are coming from) are causing a specific file to get ignored so I could easily fix the .gitignore file?
Re: [PATCH v2 2/3] commit-graph: fix buffer read-overflow
On Thu, Dec 06, 2018 at 12:20:54PM -0800, Josh Steadmon wrote: > diff --git a/commit-graph.c b/commit-graph.c > index 07dd410f3c..224a5f161e 100644 > --- a/commit-graph.c > +++ b/commit-graph.c > @@ -165,10 +165,20 @@ struct commit_graph *parse_commit_graph(void > *graph_map, int fd, > last_chunk_offset = 8; > chunk_lookup = data + 8; > for (i = 0; i < graph->num_chunks; i++) { > - uint32_t chunk_id = get_be32(chunk_lookup + 0); > - uint64_t chunk_offset = get_be64(chunk_lookup + 4); > + uint32_t chunk_id; > + uint64_t chunk_offset; > int chunk_repeated = 0; > > + if (chunk_lookup + GRAPH_CHUNKLOOKUP_WIDTH > > + data + graph_size) { > + error(_("chunk lookup table entry missing; graph file > may be incomplete")); > + free(graph); > + return NULL; > + } Is it possible to overflow the addition here? E.g., if I'm on a 32-bit system and the truncated chunk appears right at the 4GB limit, in which case we wrap back around? I guess that's pretty implausible, since it would mean that the mmap is bumping up against the end of the address space. I didn't check, but I wouldn't be surprised if sane operating systems avoid allocating those addresses. But I think you could write this as: if (data + graph_size - chunk_lookup < GRAPH_CHUNKLOOKUP_WIDTH) to avoid overflow (we know that "data + graph_size" is sane because that's our mmap, and chunk_lookup is somewhere between "data" and "data + graph_size", so the result is between 0 and graph_size). I dunno. I think I've convinced myself it's a non-issue here, but it may be good to get in the habit of writing these sorts of offset checks in an overflow-proof order. -Peff
Re: [PATCH on sb/more-repo-in-api] revision: use commit graph in get_reference()
On Thu, Dec 06, 2018 at 03:54:46PM -0800, Jonathan Tan wrote: > This makes sense - I thought I shouldn't mention the commit graph in the > code since it seems like a layering violation, but I felt the need to > mention commit graph in a comment, so maybe the need to mention commit > graph in the code is there too. Subsequently, maybe the lookup-for-type > could be replaced by a lookup-in-commit-graph (maybe by using > parse_commit_in_graph() directly), which should be at least slightly > faster. That makes more sense to me. If we don't have a commit graph at all, it's a quick noop. If we do, we might binary search in the list of commits for a non-commit. But that's strictly faster than finding the object's type (which involves a binary search of a larger list, followed by actually accessing the type info). > > In general, it would be nice if we had a more incremental API > > for accessing objects: open, get metadata, then read the data. That > > would make these kinds of optimizations "free". > > Would this be assuming that to read the data, you would (1) first need to > read the metadata, and (2) there would be no redundancy in reading the > two? It seems to me that for loose objects, you would want to perform > all your reads at once, since any read requires opening the file, and > for commit graphs, you just want to read what you want, since the > metadata and the data are in separate places. By metadata here, I don't mean the commit-graph data, but just the object type and size. So I'm imagining an interface more like: - object_open() locates the object, and stores either the pack file/offset or a descriptor to a loose path in an opaque handle struct - object_size() and object_type() on that handle would do what you expect. For loose objects, these would parse the header (the equivalent of unpack_sha1_header()). For packed ones, they'd use the object header in the pack (and chase down the delta bits as needed). - object_contents() would return the full content - object_read() could sequentially read a subset of the file (this could replace the streaming interface we currently have) We have most of the low-level bits for this already, if you poke into what object_info_extended() is doing. We just don't have them packaged in an interface which can persist across multiple calls. With an interface like that, parse_object()'s large-blob check could be something like the patch below. But your case here is a bit more interesting. If we have a commit graph, then we can avoid opening (or even finding!) the on-disk object at all. So I actually think it makes sense to just check the commit-graph first, as discussed above. --- diff --git a/object.c b/object.c index e54160550c..afce58c0bc 100644 --- a/object.c +++ b/object.c @@ -254,23 +254,31 @@ struct object *parse_object(struct repository *r, const struct object_id *oid) const struct object_id *repl = lookup_replace_object(r, oid); void *buffer; struct object *obj; + struct object_handle oh; obj = lookup_object(r, oid->hash); if (obj && obj->parsed) return obj; - if ((obj && obj->type == OBJ_BLOB && has_object_file(oid)) || - (!obj && has_object_file(oid) && -oid_object_info(r, oid, NULL) == OBJ_BLOB)) { - if (check_object_signature(repl, NULL, 0, NULL) < 0) { + if (object_open(, oid) < 0) + return NULL; /* missing object */ + + if (object_type() == OBJ_BLOB) { + /* this will call object_read() on 4k chunks */ + if (check_object_signature_stream(, oid)) { error(_("sha1 mismatch %s"), oid_to_hex(oid)); return NULL; } + object_close(); /* we don't care about contents */ parse_blob_buffer(lookup_blob(r, oid), NULL, 0); return lookup_object(r, oid->hash); } - buffer = read_object_file(oid, , ); + type = object_type(); + size = object_size(); + buffer = object_contents(); + object_close(); + if (buffer) { if (check_object_signature(repl, buffer, size, type_name(type)) < 0) { free(buffer);
Morning
Hello; kindly see the attached my proposal Charities donation. Mrs. Joy Smith Johnson.rtf Description: RTF file
HELLO DEAR
With Due Respect, I know that this mail will come to you as a surprise as we have never met before, but need not to worry as I am contacting you independently of my investigation and no one is informed of this communication. I need your urgent assistance in transferring the sum of $11.3million immediately to your private account.The money has been here in our Bank lying dormant for years now without anybody coming for the claim of it. I want to release the money to you as the relative to our deceased customer (the account owner) who died a long with his supposed NEXT OF KIN since 16th October 2005. The Banking laws here does not allow such money to stay more than 13 years, because the money will be recalled to the Bank treasury account as unclaimed fund. By indicating your interest I will send you the full details on how the business will be executed. Please respond urgently and delete if you are not interested. Best Regards, Mr.Michel Duku.