[PATCH 3/3] wt-status.c: Set the commitable flag in the collect phase.
In an update to fix a bug with "commit --dry-run" it was found that
the commitable flag was broken. The update was, at the time,
accepted as it was better than the previous version.
Since the set of the flag had been done in wt_longstatus_print_updated,
set the flag in wt_status_collect_updated_cb.
Set the commitable flag in wt_status_collect_changes_initial to keep
from introducing a rebase regression.
Leave the setting of the commitable flag in show_merge_in_progress. If
a check for merged commits is moved to the collect phase then other
--dry-run tests fail.
Signed-off-by: Stephen P. Smith
---
wt-status.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/wt-status.c b/wt-status.c
index 5ffab6101..d50798425 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -540,10 +540,12 @@ static void wt_status_collect_updated_cb(struct
diff_queue_struct *q,
/* Leave {mode,oid}_head zero for an add. */
d->mode_index = p->two->mode;
oidcpy(>oid_index, >two->oid);
+ s->commitable = 1;
break;
case DIFF_STATUS_DELETED:
d->mode_head = p->one->mode;
oidcpy(>oid_head, >one->oid);
+ s->commitable = 1;
/* Leave {mode,oid}_index zero for a delete. */
break;
@@ -561,6 +563,7 @@ static void wt_status_collect_updated_cb(struct
diff_queue_struct *q,
d->mode_index = p->two->mode;
oidcpy(>oid_head, >one->oid);
oidcpy(>oid_index, >two->oid);
+ s->commitable = 1;
break;
case DIFF_STATUS_UNMERGED:
d->stagemask = unmerged_mask(p->two->path);
@@ -665,11 +668,13 @@ static void wt_status_collect_changes_initial(struct
wt_status *s)
* code will output the stage values directly and not
use the
* values in these fields.
*/
+ s->commitable = 1;
} else {
d->index_status = DIFF_STATUS_ADDED;
/* Leave {mode,oid}_head zero for adds. */
d->mode_index = ce->ce_mode;
oidcpy(>oid_index, >oid);
+ s->commitable = 1;
}
}
}
@@ -773,7 +778,6 @@ static void wt_longstatus_print_updated(struct wt_status *s)
continue;
if (!shown_header) {
wt_longstatus_print_cached_header(s);
- s->commitable = 1;
shown_header = 1;
}
wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
--
2.18.0
[PATCH 1/3] Change tests from expecting to fail to expecting success.
Two tests were written which showed failure cases when passing --procelain or --short. Change the test to expect success since updates to the wt-status broken code section is being fixed. Signed-off-by: Stephen P. Smith --- t/t7501-commit.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh index 4cae92804..810d4cea7 100755 --- a/t/t7501-commit.sh +++ b/t/t7501-commit.sh @@ -99,12 +99,12 @@ test_expect_success '--dry-run with stuff to commit returns ok' ' git commit -m next -a --dry-run ' -test_expect_failure '--short with stuff to commit returns ok' ' +test_expect_success '--short with stuff to commit returns ok' ' echo bongo bongo bongo >>file && git commit -m next -a --short ' -test_expect_failure '--porcelain with stuff to commit returns ok' ' +test_expect_success '--porcelain with stuff to commit returns ok' ' echo bongo bongo bongo >>file && git commit -m next -a --porcelain ' -- 2.18.0
[PATCH 0/3] wt-status.c: commitable flag
A couple of years ago, during a patch review Junio found that the commitable bit as implemented in wt-status.c was broken. Stephen P. Smith (3): Change tests from expecting to fail to expecting success. Add test for commit --dry-run --short. wt-status.c: Set the commitable flag in the collect phase. t/t7501-commit.sh | 14 -- wt-status.c | 6 +- 2 files changed, 17 insertions(+), 3 deletions(-) -- 2.18.0
[PATCH 2/3] Add test for commit --dry-run --short.
Add test for commit with --dry-run --short for a new file of zero length. The test demonstrated that the setting of the commitable flag was broken as was found durning an earlier patch review. Signed-off-by: Stephen P. Smith --- t/t7501-commit.sh | 10 ++ 1 file changed, 10 insertions(+) diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh index 810d4cea7..fc69da816 100755 --- a/t/t7501-commit.sh +++ b/t/t7501-commit.sh @@ -682,4 +682,14 @@ test_expect_success '--dry-run with conflicts fixed from a merge' ' git commit -m "conflicts fixed from merge." ' +test_expect_success '--dry-run --short with conflicts fixed from a merge' ' + # setup two branches with conflicting information + # in the same file, resolve the conflict, + # call commit with --dry-run --short + rm -f test-file && + touch testfile && + git add test-file && + git commit --dry-run --short +' + test_done -- 2.18.0
Re: Automatic core.autocrlf?
Hi, Torsten Bögershausen wrote: > My original plan was to try to "make obsolete"/retire > and phase out core.autocrlf completely. > However, since e.g. egit/jgit uses it > (they don't have support for .gitattributes at all) I am not sure if this > is a good idea either. Interesting. [1] appears to have intended to add this feature. Do you remember when it is that you tested? Feel free to file bugs using [2] for any missing features. Thanks, Jonathan [1] https://git.eclipse.org/r/c/60635 [2] https://www.eclipse.org/jgit/support/
Re: Feature request: hooks directory
Ævar Arnfjörð Bjarmason wrote: > There is interest in this. This E-Mail of mine gives a good summary of > prior discussions about this: > https://public-inbox.org/git/877eqqnq22@evledraar.gmail.com/ > > I.e. it's something I've personally been interested in doing in the > past, there's various bolt-on solutions to do it (basically local hook > runners) used by various projects. A few unrelated thoughts, to expand on this. Reports of experience from using local hook runners would be very welcome so we can benefit from their good ideas and avoid their bad ones. That was part of the motivation for not building this in for so long: we want people to experiment so that the result can be something that works well for a lot of people. Separately from that, in [1] I mentioned that I want to revamp how hooks work somewhat, to avoid the attack described there (or the more common attack also described there that involves a zip file). Such a revamp would be likely to also handle this multiple-hook use case. As a word of caution, today we support having multiple credential helpers in use and it's a nightmare to support. The layering model is complicated and users don't understand it. So we might want to try to avoid whatever went wrong there. ;-) Thanks, Jonathan [1] https://public-inbox.org/git/20171002234517.gv19...@aiede.mtv.corp.google.com/
Re: Thank you for public-inbox!
Eric Wong wrote: > Jonathan Nieder wrote: >> Jeff King wrote: >>> I guess I just wonder if I set up a mirror on another domain, would >>> anybody actually _use_ it? I'd think most people would just go to >>> public-inbox.org as the de facto URL. >> >> If it's faster than public-inbox.org and you don't mind the traffic I >> would send, then I'll use it. :) > > Is performance a problem on public-inbox.org for you? It's pretty fast. I'm just very, very picky about latency. ;-) It's good to know you're interested in which corner cases are bad. The next time I have a noticeably slow page load, I'll contact meta@. [...] > I've also been sorta considering downgrading to a $5/month VPS > (from a $20/month VPS) to force myself to pay more attention to > performance while saving myself a few bucks. But I wouldn't get > to dogfood on SMP, anymore... Sounds reasonable to me. If performance gets bad, that's just a reason for people to help out (either with patches or e.g. with donated VMs for hosting). Speaking of the latter: what are your current resource requirements? E.g. which of the dimensions in [1] do you not fit into? Thanks, Jonathan [1] https://cloud.google.com/free/docs/always-free-usage-limits#compute_name
Re: [PATCH 0/9] worktree: fix bugs and broaden --force applicability
On Fri, Aug 31, 2018 at 12:49:39AM +0100, Ramsay Jones wrote:
> On 30/08/18 21:14, Junio C Hamano wrote:
> > Jeff King writes:
> >
> >> I suppose so. I don't think I've _ever_ used distclean, and I only
> >> rarely use "clean" (a testament to our Makefile's efforts to accurately
> >> track dependencies). I'd usually use "git clean" when I want something
> >> pristine (because I don't want to trust the Makefile at all).
> >
> > I do not trust "git clean" all that much, and pre-cleaning with
> > "make distclean" and then running "git clean -x" has become my bad
> > habit. I jump around quite a bit during the day, which would end up
> > littering the working tree with *.o files that are only known to one
> > but not both of {maint,pu}/Makefile's distclean rules. I even do
> > "for i in pu maint master next; do git checkout $i; make distclean; done"
> > sometimes before running "git clean -x" ;-)
> >
>
> 'git clean -x' always removes _way_ more than I want it
> to - in particular, I lost my config.mak more than once.
Heh. I have done that, too, but fortunately mine is a symlink to a copy
that is held in a git repository. ;)
-Peff
Re: [PATCH 0/9] worktree: fix bugs and broaden --force applicability
On 30/08/18 21:14, Junio C Hamano wrote:
> Jeff King writes:
>
>> I suppose so. I don't think I've _ever_ used distclean, and I only
>> rarely use "clean" (a testament to our Makefile's efforts to accurately
>> track dependencies). I'd usually use "git clean" when I want something
>> pristine (because I don't want to trust the Makefile at all).
>
> I do not trust "git clean" all that much, and pre-cleaning with
> "make distclean" and then running "git clean -x" has become my bad
> habit. I jump around quite a bit during the day, which would end up
> littering the working tree with *.o files that are only known to one
> but not both of {maint,pu}/Makefile's distclean rules. I even do
> "for i in pu maint master next; do git checkout $i; make distclean; done"
> sometimes before running "git clean -x" ;-)
>
'git clean -x' always removes _way_ more than I want it
to - in particular, I lost my config.mak more than once.
So, no I don't trust it. ;-)
ATB,
Ramsay Jones
Re: [PATCH v4 4/6] push doc: correct lies about how push refspecs work
On Thu, Aug 30 2018, Ævar Arnfjörð Bjarmason wrote:
[Notes to self]
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index 55277a9781..0f03d36f1e 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -74,12 +74,41 @@ without any `` on the command line. Otherwise,
> missing
> `:` means to update the same ref as the ``.
> +
> The object referenced by is used to update the reference
> -on the remote side. By default this is only allowed if is not
> -a tag (annotated or lightweight), and then only if it can fast-forward
> -. By having the optional leading `+`, you can tell Git to update
> -the ref even if it is not allowed by default (e.g., it is not a
> -fast-forward.) This does *not* attempt to merge into . See
> -EXAMPLES below for details.
> +on the remote side. Whether this is allowed depends on where in
> +`refs/*` the reference lives as described in detail below. Any
> +such update does *not* attempt to merge into . See EXAMPLES
> +below for details.
> ++
> +The `refs/heads/*` namespace will only accept commit objects, and only
> +if they can be fast-forwarded.
> ++
> +The `refs/tags/*` namespace will accept any kind of object (as
> +commits, trees and blobs can be tagged), and any changes to them will
> +be rejected.
> ++
Both of these should carve out some mention for the "deletion" aspect of
"updates". I.e. you don't need --force to delete.
> +It's possible to push any type of object to any namespace outside of
> +`refs/{tags,heads}/*`. In the case of tags and commits, these will be
> +treated as if they were the commits inside `refs/heads/*` for the
> +purposes of whether the update is allowed.
> ++
> +I.e. a fast-forward of commits and tags outside `refs/{tags,heads}/*`
> +is allowed, even in cases where what's being fast-forwarded is not a
> +commit, but a tag object which happens to point to a new commit which
> +is a fast-forward of the commit the last tag (or commit) it's
> +replacing. Replacing a tag with an entirely different tag is also
> +allowed, if it points to the same commit, as well as pushing a peeled
> +tag, i.e. pushing the commit that existing tag object points to, or a
> +new tag object which an existing commit points to.
> ++
> +Tree and blob objects outside of `refs/{tags,heads}/*` will be treated
> +the same way as if they were inside `refs/tags/*`, any modification of
> +them will be rejected.
> ++
> +All of the rules described above about what's not allowed as an update
> +can be overridden by adding an the optional leading `+` to a refspec
> +(or using `--force` command line option). The only exception to this
> +is that no amount of forcing will make the `refs/heads/*` namespace
> +accept a non-commit object.
> +
> `tag ` means the same as `refs/tags/:refs/tags/`.
> +
Later below this we say:
Pushing an empty allows you to delete the ref from the
remote repository.
Which, perhaps given the discussion of deletions as updates, should be
mentioned earlier in some way, i.e. should we just say above all these
rules that by "update" we mean non-deletions?
Re: [GSoC][PATCH v8 00/20] Convert "git stash" to C builtin
On Thu, Aug 30 2018, Paul-Sebastian Ungureanu wrote:
> Hello,
>
> This a new iteration of `stash.c`. What is new?
>
> * Some commits got squashed. The commit related to replacing
> `git apply` child process was dropped since it wasn't the best
> idea.
>
> * In v7, there was a bug [1] related to config `git stash show`
> The bug was fixed and a test file was added for this.
>
> * Fixed `git stash [push]` [2]. In v7, `git stash -q drop` would
> act like `git stash push -q drop`.
>
> * Fixed coding-style nits. Verified that messages are marked
> for translation and are going to the correct output stream.
>
> * Fixed one memory leak (related to `strbuf_detach`).
>
> * Simplified the code a little bit.
This looks good, I read this and a previous version. I'e tested this on
Linux, FreeBSD & AIX. All tests pass on all of them.
One style nit: These patches consistently indent wrapped lines not to
align with the opening "(" (as is our usual style), but just with
"\n\t". I.e. this patch on top would make it like what we usually do
(but should be squashed as appropriate):
diff --git a/builtin/stash.c b/builtin/stash.c
index dd1084afd4..6d849ed811 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -389,3 +389,3 @@ static int restore_untracked(struct object_id *u_tree)
static int do_apply_stash(const char *prefix, struct stash_info *info,
- int index)
+ int index)
{
@@ -408,3 +408,3 @@ static int do_apply_stash(const char *prefix, struct
stash_info *info,
if (!oidcmp(>b_tree, >i_tree) || !oidcmp(_tree,
- >i_tree)) {
+
>i_tree)) {
has_index = 0;
@@ -514,3 +514,3 @@ static int apply_stash(int argc, const char **argv,
const char *prefix)
OPT_BOOL(0, "index", ,
- N_("attempt to recreate the index")),
+N_("attempt to recreate the index")),
OPT_END()
@@ -610,3 +610,3 @@ static int pop_stash(int argc, const char **argv, const
char *prefix)
OPT_BOOL(0, "index", ,
- N_("attempt to recreate the index")),
+N_("attempt to recreate the index")),
OPT_END()
@@ -971,3 +971,3 @@ static int stash_patch(struct stash_info *info, struct
pathspec ps, int quiet)
argv_array_pushl(_add_i.args, "add--interactive", "--patch=stash",
- "--", NULL);
+"--", NULL);
for (i = 0; i < ps.nr; ++i)
@@ -1279,3 +1279,3 @@ static int do_push_stash(struct pathspec ps, const
char *stash_msg, int quiet,
printf_ln(_("Saved working directory and index state %s"),
- stash_msg);
+ stash_msg);
@@ -1413,5 +1413,5 @@ static int push_stash(int argc, const char **argv,
const char *prefix)
OPT_SET_INT('k', "keep-index", _index,
- N_("keep index"), 1),
+ N_("keep index"), 1),
OPT_BOOL('p', "patch", _mode,
- N_("stash in patch mode")),
+N_("stash in patch mode")),
OPT__QUIET(, N_("quiet mode")),
@@ -1422,3 +1422,3 @@ static int push_stash(int argc, const char **argv,
const char *prefix)
OPT_STRING('m', "message", _msg, N_("message"),
-N_("stash message")),
+ N_("stash message")),
OPT_END()
@@ -1450,5 +1450,5 @@ static int save_stash(int argc, const char **argv,
const char *prefix)
OPT_SET_INT('k', "keep-index", _index,
- N_("keep index"), 1),
+ N_("keep index"), 1),
OPT_BOOL('p', "patch", _mode,
- N_("stash in patch mode")),
+N_("stash in patch mode")),
OPT__QUIET(, N_("quiet mode")),
Tip: It's also better to get feedback by CC-ing people who've had
feedback on previous versions.
Re: [GSoC][PATCH v8 03/20] stash: update test cases conform to coding guidelines
Paul-Sebastian Ungureanu writes: > Removed whitespaces after redirection operators. A clean-up that is long overdue. Thanks. The result still has some rooms to improve (e.g. the preparation of 'expect' file probably should be done in a test_expect_success block of the setup step; some are called 'expect' while others are called 'expected'; some test_cmp invocations do not have expected output as their first argument), but that can be done later.
Re: [GSoC][PATCH v8 05/20] stash: add tests for `git stash show` config
Paul-Sebastian Ungureanu writes:
> This commit introduces tests for `git stash show`
> config. It tests all the cases where `stash.showStat`
> and `stash.showPatch` are unset or set to true / false.
>
> Signed-off-by: Paul-Sebastian Ungureanu
> ---
> t/t3907-stash-show-config.sh | 81
> 1 file changed, 81 insertions(+)
> create mode 100755 t/t3907-stash-show-config.sh
>
> diff --git a/t/t3907-stash-show-config.sh b/t/t3907-stash-show-config.sh
> new file mode 100755
> index 00..8fe369c1a1
> --- /dev/null
> +++ b/t/t3907-stash-show-config.sh
> @@ -0,0 +1,81 @@
> +#!/bin/sh
> +
> +test_description='Test git stash show configuration.'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> + test_commit file
> +'
> +
> +# takes three parameters:
> +# 1. the stash.showStat value (or "")
> +# 2. the stash.showPatch value (or "")
> +# 3. the diff options of the expected output (or nothing for no output)
> +test_stat_and_patch () {
> + if test "" = "$1"
> + then
> + test_might_fail git config --unset stash.showStat
> + else
> + test_config stash.showStat "$1"
> + fi &&
> +
> + if test "" = "$2"
> + then
> + test_might_fail git config --unset stash.showPatch
I think you are trying to protect yourself from an error triggered
by unsetting what is not set, but for that, test_unconfig is
probably a better choice, as it still catches errors of other types
and ignores only that "unset a variable that is not set" error.
> + else
> + test_config stash.showPatch "$2"
> + fi &&
> +
> + shift &&
> + shift &&
You can use "shift 2 &&" here (not worth a reroll).
> + echo 2 >file.t &&
> + git diff "$@" >expect &&
When the caller does not give $3 to this function, it does not look
at 'expect'. I think it is clearer if you did
if test $# != 0
then
git diff "$@" >expect
fi &&
here, and ...
> + git stash &&
> + git stash show >actual &&
> +
> + if test -z "$1"
... wrote this as
if test $# = 0
The only difference between '-z "$1"' and '$# = 0' is when he caller
passes an empty string to the function as $3, which you never do, so
the distinction is theoretical, but using $# makes your intention
clear that you do not mean to treat an empty string any specially.
> + then
> + test_must_be_empty actual
> + else
> + test_cmp expect actual
> + fi
> +}
> +
> +test_expect_success 'showStat unset showPatch unset' '
> + test_stat_and_patch "" "" --stat
> +'
> +
> +test_expect_success 'showStat unset showPatch false' '
> + test_stat_and_patch "" false --stat
> +'
> +
> +test_expect_success 'showStat unset showPatch true' '
> + test_stat_and_patch "" true --stat -p
> +'
> +
> +test_expect_success 'showStat false showPatch unset' '
> + test_stat_and_patch false ""
> +'
> +
> +test_expect_success 'showStat false showPatch false' '
> + test_stat_and_patch false false
> +'
> +
> +test_expect_success 'showStat false showPatch true' '
> + test_stat_and_patch false true -p
> +'
> +
> +test_expect_success 'showStat true showPatch unset' '
> + test_stat_and_patch true "" --stat
> +'
> +
> +test_expect_success 'showStat true showPatch false' '
> + test_stat_and_patch true false --stat
> +'
> +
> +test_expect_success 'showStat true showPatch true' '
> + test_stat_and_patch true true --stat -p
> +'
> +
> +test_done
Re: [PATCH v4 6/6] fetch: stop clobbering existing tags without --force
Ævar Arnfjörð Bjarmason writes:
> +
> -Unlike when pushing with linkgit:git-push[1], any updates to
> -`refs/tags/*` will be accepted without `+` in the refspec (or
> -`--force`). The receiving promiscuously considers all tag updates from
> -a remote to be forced fetches.
> +Until Git version 2.20, and unlike when pushing with
> +linkgit:git-push[1], any updates to `refs/tags/*` would be accepted
> +without `+` in the refspec (or `--force`). The receiving promiscuously
> +considered all tag updates from a remote to be forced fetches. Since
> +Git version 2.20 updates to `refs/tags/*` work the same way as when
> +pushing. I.e. any updates will be rejected without `+` in the refspec
> +(or `--force`).
Have a comma after 2.20; otherwise it was unreadable, at least to
me, who took three attempts before realizing that the "updates" is
not a verb whose subject is "Git version 2.20". Or
Since Git version 2.20, fetching to update `refs/tags/*`
work the same way as pushing into it
perhaps.
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index b0706b3803..ed4ed9d8c4 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -667,12 +667,18 @@ static int update_local_ref(struct ref *ref,
>
> if (!is_null_oid(>old_oid) &&
> starts_with(ref->name, "refs/tags/")) {
> - int r;
> - r = s_update_ref("updating tag", ref, 0);
> - format_display(display, r ? '!' : 't', _("[tag update]"),
> -r ? _("unable to update local ref") : NULL,
> -remote, pretty_ref, summary_width);
> - return r;
> + if (force || ref->force) {
> + int r;
> + r = s_update_ref("updating tag", ref, 0);
> + format_display(display, r ? '!' : 't', _("[tag
> update]"),
> +r ? _("unable to update local ref") :
> NULL,
> +remote, pretty_ref, summary_width);
> + return r;
> + } else {
> + format_display(display, '!', _("[rejected]"), _("would
> clobber existing tag"),
> +remote, pretty_ref, summary_width);
> + return 1;
> + }
> }
A straight-forward change to turn an unconditional update to either
an unconditonal rejection (when force is not given) or an
unconditional acceptance (when forced), which makes sense and has
near-zero chance of being wrong ;-)
It is a huge change in behaviour, but in a very good way. I'd
imagine that users will welcome it very much.
[GSoC][PATCH v8 13/20] stash: convert store to builtin
Add stash store to the helper and delete the store_stash function
from the shell script.
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 55 +
git-stash.sh| 43 ++--
2 files changed, 57 insertions(+), 41 deletions(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 02b593e0cd..87568b0f34 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -58,6 +58,11 @@ static const char * const git_stash_helper_clear_usage[] = {
NULL
};
+static const char * const git_stash_helper_store_usage[] = {
+ N_("git stash--helper store [-m|--message ] [-q|--quiet]
"),
+ NULL
+};
+
static const char *ref_stash = "refs/stash";
static int quiet;
static struct strbuf stash_index_path = STRBUF_INIT;
@@ -723,6 +728,54 @@ static int show_stash(int argc, const char **argv, const
char *prefix)
return diff_result_code(, 0);
}
+static int do_store_stash(const char *w_commit, const char *stash_msg,
+ int quiet)
+{
+ int ret = 0;
+ int need_to_free = 0;
+ struct object_id obj;
+
+ if (!stash_msg) {
+ need_to_free = 1;
+ stash_msg = xstrdup("Created via \"git stash store\".");
+ }
+
+ ret = get_oid(w_commit, );
+ if (!ret) {
+ ret = update_ref(stash_msg, ref_stash, , NULL,
+REF_FORCE_CREATE_REFLOG,
+quiet ? UPDATE_REFS_QUIET_ON_ERR :
+UPDATE_REFS_MSG_ON_ERR);
+ }
+ if (ret && !quiet)
+ fprintf_ln(stderr, _("Cannot update %s with %s"),
+ ref_stash, w_commit);
+ if (need_to_free)
+ free((char *) stash_msg);
+ return ret;
+}
+
+static int store_stash(int argc, const char **argv, const char *prefix)
+{
+ const char *stash_msg = NULL;
+ struct option options[] = {
+ OPT__QUIET(, N_("be quiet, only report errors")),
+ OPT_STRING('m', "message", _msg, "message", N_("stash
message")),
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_store_usage,
+PARSE_OPT_KEEP_UNKNOWN);
+
+ if (argc != 1) {
+ fprintf_ln(stderr, _("\"git stash store\" requires one
argument"));
+ return -1;
+ }
+
+ return do_store_stash(argv[0], stash_msg, quiet);
+}
+
int cmd_stash__helper(int argc, const char **argv, const char *prefix)
{
pid_t pid = getpid();
@@ -757,6 +810,8 @@ int cmd_stash__helper(int argc, const char **argv, const
char *prefix)
return !!list_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "show"))
return !!show_stash(argc, argv, prefix);
+ else if (!strcmp(argv[0], "store"))
+ return !!store_stash(argc, argv, prefix);
usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 0d05cbc1e5..5739c51527 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -191,45 +191,6 @@ create_stash () {
die "$(gettext "Cannot record working tree state")"
}
-store_stash () {
- while test $# != 0
- do
- case "$1" in
- -m|--message)
- shift
- stash_msg="$1"
- ;;
- -m*)
- stash_msg=${1#-m}
- ;;
- --message=*)
- stash_msg=${1#--message=}
- ;;
- -q|--quiet)
- quiet=t
- ;;
- *)
- break
- ;;
- esac
- shift
- done
- test $# = 1 ||
- die "$(eval_gettext "\"$dashless store\" requires one
argument")"
-
- w_commit="$1"
- if test -z "$stash_msg"
- then
- stash_msg="Created via \"git stash store\"."
- fi
-
- git update-ref --create-reflog -m "$stash_msg" $ref_stash $w_commit
- ret=$?
- test $ret != 0 && test -z "$quiet" &&
- die "$(eval_gettext "Cannot update \$ref_stash with \$w_commit")"
- return $ret
-}
-
push_stash () {
keep_index=
patch_mode=
@@ -308,7 +269,7 @@ push_stash () {
clear_stash || die "$(gettext "Cannot initialize stash")"
create_stash -m "$stash_msg" -u "$untracked" -- "$@"
- store_stash -m "$stash_msg" -q $w_commit ||
+ git stash--helper store -m "$stash_msg" -q $w_commit ||
die "$(gettext "Cannot save the current status")"
say "$(eval_gettext "Saved working directory and index state
\$stash_msg")"
@@
[GSoC][PATCH v8 14/20] stash: convert create to builtin
Add stash create to the helper.
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 430
git-stash.sh| 2 +-
2 files changed, 431 insertions(+), 1 deletion(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 87568b0f34..ce360a569d 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -12,6 +12,7 @@
#include "rerere.h"
#include "revision.h"
#include "log-tree.h"
+#include "diffcore.h"
static const char * const git_stash_helper_usage[] = {
N_("git stash--helper list []"),
@@ -63,6 +64,11 @@ static const char * const git_stash_helper_store_usage[] = {
NULL
};
+static const char * const git_stash_helper_create_usage[] = {
+ N_("git stash--helper create []"),
+ NULL
+};
+
static const char *ref_stash = "refs/stash";
static int quiet;
static struct strbuf stash_index_path = STRBUF_INIT;
@@ -290,6 +296,18 @@ static int reset_head(void)
return run_command();
}
+static void add_diff_to_buf(struct diff_queue_struct *q,
+ struct diff_options *options,
+ void *data)
+{
+ int i;
+ for (i = 0; i < q->nr; i++) {
+ struct diff_filepair *p = q->queue[i];
+ strbuf_addstr(data, p->one->path);
+ strbuf_addch(data, 0);
+ }
+}
+
static int get_newly_staged(struct strbuf *out, struct object_id *c_tree)
{
struct child_process cp = CHILD_PROCESS_INIT;
@@ -776,6 +794,416 @@ static int store_stash(int argc, const char **argv, const
char *prefix)
return do_store_stash(argv[0], stash_msg, quiet);
}
+/*
+ * `out` will be filled with the names of untracked files. The return value is:
+ *
+ * = 0 if there are not any untracked files
+ * > 0 if there are untracked files
+ */
+static int get_untracked_files(struct pathspec ps, int include_untracked,
+ struct strbuf *out)
+{
+ int max_len;
+ int i;
+ char *seen;
+ struct dir_struct dir;
+
+ memset(, 0, sizeof(dir));
+ if (include_untracked != 2)
+ setup_standard_excludes();
+
+ seen = xcalloc(ps.nr, 1);
+
+ max_len = fill_directory(, the_repository->index, );
+ for (i = 0; i < dir.nr; i++) {
+ struct dir_entry *ent = dir.entries[i];
+ if (!dir_path_match(_index, ent, , max_len, seen)) {
+ free(ent);
+ continue;
+ }
+ strbuf_addf(out, "%s%c", ent->name, '\0');
+ free(ent);
+ }
+
+ free(dir.entries);
+ free(dir.ignored);
+ clear_directory();
+ free(seen);
+ return out->len;
+}
+
+/*
+ * The return value of `check_changes()` can be:
+ *
+ * < 0 if there was an error
+ * = 0 if there are no changes.
+ * > 0 if there are changes.
+ */
+static int check_changes(struct pathspec ps, int include_untracked)
+{
+ int result;
+ int ret = 0;
+ struct rev_info rev;
+ struct object_id dummy;
+ struct strbuf out = STRBUF_INIT;
+
+ init_revisions(, NULL);
+ rev.prune_data = ps;
+
+ rev.diffopt.flags.quick = 1;
+ rev.diffopt.flags.ignore_submodules = 1;
+ rev.abbrev = 0;
+
+ /* No initial commit. */
+ if (get_oid("HEAD", ))
+ return -1;
+
+ add_head_to_pending();
+ diff_setup_done();
+
+ if (read_cache() < 0)
+ return 1;
+ result = run_diff_index(, 1);
+ if (diff_result_code(, result))
+ return 1;
+
+ object_array_clear();
+ result = run_diff_files(, 0);
+ if (diff_result_code(, result))
+ return 1;
+
+ if (include_untracked && get_untracked_files(ps, include_untracked,
+)) {
+ strbuf_release();
+ return 1;
+ }
+
+ strbuf_release();
+ return 0;
+}
+
+static int save_untracked_files(struct stash_info *info, struct strbuf *msg,
+ struct strbuf *in)
+{
+ int ret = 0;
+ struct strbuf untracked_msg = STRBUF_INIT;
+ struct strbuf out = STRBUF_INIT;
+ struct child_process cp_upd_index = CHILD_PROCESS_INIT;
+ struct child_process cp_write_tree = CHILD_PROCESS_INIT;
+
+ cp_upd_index.git_cmd = 1;
+ argv_array_pushl(_upd_index.args, "update-index", "-z", "--add",
+"--remove", "--stdin", NULL);
+ argv_array_pushf(_upd_index.env_array, "GIT_INDEX_FILE=%s",
+stash_index_path.buf);
+
+ strbuf_addf(_msg, "untracked files on %s\n", msg->buf);
+ if (pipe_command(_upd_index, in->buf, in->len, NULL, 0, NULL, 0)) {
+ ret = -1;
+ goto done;
+ }
+
+ cp_write_tree.git_cmd = 1;
+ argv_array_push(_write_tree.args, "write-tree");
+
[GSoC][PATCH v8 17/20] stash: convert save to builtin
Add stash save to the helper and delete functions which are no
longer needed (`show_help()`, `save_stash()`, `push_stash()`,
`create_stash()`, `clear_stash()`, `untracked_files()` and
`no_changes()`).
The `-m` option is no longer supported as it might not make
sense to have two ways of passing a message. Even if this is
a change in behaviour, the documentation remains the same
because the `-m` parameter was omitted before.
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 52 +++
git-stash.sh| 311 +---
2 files changed, 54 insertions(+), 309 deletions(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index e5153a63ea..1269f2548c 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -24,6 +24,8 @@ static const char * const git_stash_helper_usage[] = {
N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index]
[-q|--quiet]\n"
" [-u|--include-untracked] [-a|--all] [-m|--message
]\n"
" [--] [...]]"),
+ N_("git stash--helper save [-p|--patch] [-k|--[no-]keep-index]
[-q|--quiet]\n"
+ " [-u|--include-untracked] [-a|--all] []"),
NULL
};
@@ -79,6 +81,12 @@ static const char * const git_stash_helper_push_usage[] = {
NULL
};
+static const char * const git_stash_helper_save_usage[] = {
+ N_("git stash--helper save [-p|--patch] [-k|--[no-]keep-index]
[-q|--quiet]\n"
+ " [-u|--include-untracked] [-a|--all] []"),
+ NULL
+};
+
static const char *ref_stash = "refs/stash";
static int quiet;
static struct strbuf stash_index_path = STRBUF_INIT;
@@ -1444,6 +1452,48 @@ static int push_stash(int argc, const char **argv, const
char *prefix)
include_untracked);
}
+static int save_stash(int argc, const char **argv, const char *prefix)
+{
+ int i;
+ int keep_index = -1;
+ int patch_mode = 0;
+ int include_untracked = 0;
+ int quiet = 0;
+ int ret = 0;
+ const char *stash_msg = NULL;
+ char *to_free = NULL;
+ struct strbuf stash_msg_buf = STRBUF_INIT;
+ struct pathspec ps;
+ struct option options[] = {
+ OPT_SET_INT('k', "keep-index", _index,
+ N_("keep index"), 1),
+ OPT_BOOL('p', "patch", _mode,
+ N_("stash in patch mode")),
+ OPT__QUIET(, N_("quiet mode")),
+ OPT_BOOL('u', "include-untracked", _untracked,
+N_("include untracked files in stash")),
+ OPT_SET_INT('a', "all", _untracked,
+ N_("include ignore files"), 2),
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_save_usage,
+0);
+
+ for (i = 0; i < argc; ++i)
+ strbuf_addf(_msg_buf, "%s ", argv[i]);
+ stash_msg = strbuf_detach(_msg_buf, NULL);
+ to_free = (char *) stash_msg;
+
+ memset(, 0, sizeof(ps));
+ ret = do_push_stash(ps, stash_msg, quiet, keep_index, patch_mode,
+ include_untracked);
+
+ free(to_free);
+ return ret;
+}
+
int cmd_stash__helper(int argc, const char **argv, const char *prefix)
{
pid_t pid = getpid();
@@ -1484,6 +1534,8 @@ int cmd_stash__helper(int argc, const char **argv, const
char *prefix)
return !!create_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "push"))
return !!push_stash(argc, argv, prefix);
+ else if (!strcmp(argv[0], "save"))
+ return !!save_stash(argc, argv, prefix);
usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index c3146f62ab..695f1feba3 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -36,314 +36,6 @@ else
reset_color=
fi
-no_changes () {
- git diff-index --quiet --cached HEAD --ignore-submodules -- "$@" &&
- git diff-files --quiet --ignore-submodules -- "$@" &&
- (test -z "$untracked" || test -z "$(untracked_files "$@")")
-}
-
-untracked_files () {
- if test "$1" = "-z"
- then
- shift
- z=-z
- else
- z=
- fi
- excl_opt=--exclude-standard
- test "$untracked" = "all" && excl_opt=
- git ls-files -o $z $excl_opt -- "$@"
-}
-
-clear_stash () {
- if test $# != 0
- then
- die "$(gettext "git stash clear with parameters is
unimplemented")"
- fi
- if current=$(git rev-parse --verify --quiet $ref_stash)
- then
- git update-ref -d $ref_stash $current
- fi
-}
-
-create_stash () {
- stash_msg=
- untracked=
- while test $# != 0
- do
- case "$1"
[GSoC][PATCH v8 16/20] stash: make push -q quiet
There is a change in behaviour with this commit. When there was
no initial commit, the shell version of stash would still display
a message. This commit makes `push` to not display any message if
`--quiet` or `-q` is specified.
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 47 ++---
t/t3903-stash.sh| 23
2 files changed, 53 insertions(+), 17 deletions(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 23670321d8..e5153a63ea 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -938,7 +938,7 @@ static int save_untracked_files(struct stash_info *info,
struct strbuf *msg,
static struct strbuf patch = STRBUF_INIT;
-static int stash_patch(struct stash_info *info, struct pathspec ps)
+static int stash_patch(struct stash_info *info, struct pathspec ps, int quiet)
{
int i;
int ret = 0;
@@ -991,7 +991,8 @@ static int stash_patch(struct stash_info *info, struct
pathspec ps)
}
if (!patch.len) {
- fprintf_ln(stderr, _("No changes selected"));
+ if (!quiet)
+ fprintf_ln(stderr, _("No changes selected"));
ret = 1;
}
@@ -1069,7 +1070,7 @@ static int stash_working_tree(struct stash_info *info,
struct pathspec ps)
static int do_create_stash(struct pathspec ps, const char **stash_msg,
int include_untracked, int patch_mode,
- struct stash_info *info)
+ struct stash_info *info, int quiet)
{
int untracked_commit_option = 0;
int ret = 0;
@@ -1094,7 +1095,8 @@ static int do_create_stash(struct pathspec ps, const char
**stash_msg,
}
if (get_oid("HEAD", >b_commit)) {
- fprintf_ln(stderr, _("You do not have the initial commit yet"));
+ if (!quiet)
+ fprintf_ln(stderr, _("You do not have the initial
commit yet"));
ret = -1;
*stash_msg = NULL;
goto done;
@@ -1115,7 +1117,8 @@ static int do_create_stash(struct pathspec ps, const char
**stash_msg,
if (write_cache_as_tree(>i_tree, 0, NULL) ||
commit_tree(commit_tree_label.buf, commit_tree_label.len,
>i_tree, parents, >i_commit, NULL, NULL)) {
- fprintf_ln(stderr, _("Cannot save the current index state"));
+ if (!quiet)
+ fprintf_ln(stderr, _("Cannot save the current index
state"));
ret = -1;
*stash_msg = NULL;
goto done;
@@ -1124,7 +1127,8 @@ static int do_create_stash(struct pathspec ps, const char
**stash_msg,
if (include_untracked && get_untracked_files(ps, include_untracked,
)) {
if (save_untracked_files(info, , )) {
- fprintf_ln(stderr, _("Cannot save the untracked
files"));
+ if (!quiet)
+ fprintf_ln(stderr, _("Cannot save the untracked
files"));
ret = -1;
*stash_msg = NULL;
goto done;
@@ -1132,17 +1136,19 @@ static int do_create_stash(struct pathspec ps, const
char **stash_msg,
untracked_commit_option = 1;
}
if (patch_mode) {
- ret = stash_patch(info, ps);
+ ret = stash_patch(info, ps, quiet);
*stash_msg = NULL;
if (ret < 0) {
- fprintf_ln(stderr, _("Cannot save the current worktree
state"));
+ if (!quiet)
+ fprintf_ln(stderr, _("Cannot save the current
worktree state"));
goto done;
} else if (ret > 0) {
goto done;
}
} else {
if (stash_working_tree(info, ps)) {
- fprintf_ln(stderr, _("Cannot save the current worktree
state"));
+ if (!quiet)
+ fprintf_ln(stderr, _("Cannot save the current
worktree state"));
ret = -1;
*stash_msg = NULL;
goto done;
@@ -1168,7 +1174,8 @@ static int do_create_stash(struct pathspec ps, const char
**stash_msg,
if (commit_tree(*stash_msg, strlen(*stash_msg), >w_tree,
parents, >w_commit, NULL, NULL)) {
- fprintf_ln(stderr, _("Cannot record working tree state"));
+ if (!quiet)
+ fprintf_ln(stderr, _("Cannot record working tree
state"));
ret = -1;
goto done;
}
@@ -1201,7 +1208,7 @@ static int create_stash(int argc, const char **argv,
const char *prefix)
0);
[GSoC][PATCH v8 18/20] stash: convert `stash--helper.c` into `stash.c`
The old shell script `git-stash.sh` was removed and replaced
entirely by `builtin/stash.c`. In order to do that, `create` and
`push` were adapted to work without `stash.sh`. For example, before
this commit, `git stash create` called `git stash--helper create
--message "$*"`. If it called `git stash--helper create "$@"`, then
some of these changes wouldn't have been necessary.
This commit also removes the word `helper` since now stash is
called directly and not by a shell script.
Signed-off-by: Paul-Sebastian Ungureanu
---
.gitignore | 1 -
Makefile | 3 +-
builtin.h| 2 +-
builtin/{stash--helper.c => stash.c} | 161 +--
git-stash.sh | 153 -
git.c| 2 +-
6 files changed, 106 insertions(+), 216 deletions(-)
rename builtin/{stash--helper.c => stash.c} (90%)
delete mode 100755 git-stash.sh
diff --git a/.gitignore b/.gitignore
index b59661cb88..ffceea7d59 100644
--- a/.gitignore
+++ b/.gitignore
@@ -157,7 +157,6 @@
/git-show-ref
/git-stage
/git-stash
-/git-stash--helper
/git-status
/git-stripspace
/git-submodule
diff --git a/Makefile b/Makefile
index f900c68e69..ac1787d113 100644
--- a/Makefile
+++ b/Makefile
@@ -617,7 +617,6 @@ SCRIPT_SH += git-quiltimport.sh
SCRIPT_SH += git-rebase.sh
SCRIPT_SH += git-remote-testgit.sh
SCRIPT_SH += git-request-pull.sh
-SCRIPT_SH += git-stash.sh
SCRIPT_SH += git-submodule.sh
SCRIPT_SH += git-web--browse.sh
@@ -1093,7 +1092,7 @@ BUILTIN_OBJS += builtin/shortlog.o
BUILTIN_OBJS += builtin/show-branch.o
BUILTIN_OBJS += builtin/show-index.o
BUILTIN_OBJS += builtin/show-ref.o
-BUILTIN_OBJS += builtin/stash--helper.o
+BUILTIN_OBJS += builtin/stash.o
BUILTIN_OBJS += builtin/stripspace.o
BUILTIN_OBJS += builtin/submodule--helper.o
BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index 317bc338f7..e60f0fc58f 100644
--- a/builtin.h
+++ b/builtin.h
@@ -223,7 +223,7 @@ extern int cmd_show(int argc, const char **argv, const char
*prefix);
extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
extern int cmd_show_index(int argc, const char **argv, const char *prefix);
extern int cmd_status(int argc, const char **argv, const char *prefix);
-extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);
+extern int cmd_stash(int argc, const char **argv, const char *prefix);
extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
extern int cmd_submodule__helper(int argc, const char **argv, const char
*prefix);
extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash--helper.c b/builtin/stash.c
similarity index 90%
rename from builtin/stash--helper.c
rename to builtin/stash.c
index 1269f2548c..3d2316e3f7 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash.c
@@ -14,75 +14,75 @@
#include "log-tree.h"
#include "diffcore.h"
-static const char * const git_stash_helper_usage[] = {
- N_("git stash--helper list []"),
- N_("git stash--helper show [] []"),
- N_("git stash--helper drop [-q|--quiet] []"),
- N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet]
[]"),
- N_("git stash--helper branch []"),
- N_("git stash--helper clear"),
- N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index]
[-q|--quiet]\n"
+static const char * const git_stash_usage[] = {
+ N_("git stash list []"),
+ N_("git stash show [] []"),
+ N_("git stash drop [-q|--quiet] []"),
+ N_("git stash ( pop | apply ) [--index] [-q|--quiet] []"),
+ N_("git stash branch []"),
+ N_("git stash clear"),
+ N_("git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
" [-u|--include-untracked] [-a|--all] [-m|--message
]\n"
" [--] [...]]"),
- N_("git stash--helper save [-p|--patch] [-k|--[no-]keep-index]
[-q|--quiet]\n"
+ N_("git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
" [-u|--include-untracked] [-a|--all] []"),
NULL
};
-static const char * const git_stash_helper_list_usage[] = {
- N_("git stash--helper list []"),
+static const char * const git_stash_list_usage[] = {
+ N_("git stash list []"),
NULL
};
-static const char * const git_stash_helper_show_usage[] = {
- N_("git stash--helper show [] []"),
+static const char * const git_stash_show_usage[] = {
+ N_("git stash show [] []"),
NULL
};
-static const char * const git_stash_helper_drop_usage[] = {
- N_("git stash--helper drop [-q|--quiet] []"),
+static const char * const git_stash_drop_usage[] = {
+ N_("git stash drop [-q|--quiet] []"),
NULL
};
-static const char * const git_stash_helper_pop_usage[] = {
- N_("git stash--helper pop [--index]
[GSoC][PATCH v8 20/20] stash: replace all `write-tree` child processes with API calls
This commit replaces spawning `git write-tree` with API calls.
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash.c | 41 -
1 file changed, 12 insertions(+), 29 deletions(-)
diff --git a/builtin/stash.c b/builtin/stash.c
index ba5818e24e..dd1084afd4 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -910,9 +910,8 @@ static int save_untracked_files(struct stash_info *info,
struct strbuf *msg)
{
int ret = 0;
struct strbuf untracked_msg = STRBUF_INIT;
- struct strbuf out = STRBUF_INIT;
struct child_process cp_upd_index = CHILD_PROCESS_INIT;
- struct child_process cp_write_tree = CHILD_PROCESS_INIT;
+ struct index_state istate = { NULL };
cp_upd_index.git_cmd = 1;
argv_array_pushl(_upd_index.args, "update-index", "-z", "--add",
@@ -927,15 +926,11 @@ static int save_untracked_files(struct stash_info *info,
struct strbuf *msg)
goto done;
}
- cp_write_tree.git_cmd = 1;
- argv_array_push(_write_tree.args, "write-tree");
- argv_array_pushf(_write_tree.env_array, "GIT_INDEX_FILE=%s",
-stash_index_path.buf);
- if (pipe_command(_write_tree, NULL, 0, , 0,NULL, 0)) {
+ if (write_index_as_tree(>u_tree, , stash_index_path.buf, 0,
+ NULL)) {
ret = -1;
goto done;
}
- get_oid_hex(out.buf, >u_tree);
if (commit_tree(untracked_msg.buf, untracked_msg.len,
>u_tree, NULL, >u_commit, NULL, NULL)) {
@@ -944,8 +939,8 @@ static int save_untracked_files(struct stash_info *info,
struct strbuf *msg)
}
done:
+ discard_index();
strbuf_release(_msg);
- strbuf_release();
remove_path(stash_index_path.buf);
return ret;
}
@@ -956,11 +951,10 @@ static int stash_patch(struct stash_info *info, struct
pathspec ps, int quiet)
{
int i;
int ret = 0;
- struct strbuf out = STRBUF_INIT;
struct child_process cp_read_tree = CHILD_PROCESS_INIT;
struct child_process cp_add_i = CHILD_PROCESS_INIT;
- struct child_process cp_write_tree = CHILD_PROCESS_INIT;
struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
+ struct index_state istate = { NULL };
remove_path(stash_index_path.buf);
@@ -985,17 +979,12 @@ static int stash_patch(struct stash_info *info, struct
pathspec ps, int quiet)
goto done;
}
- cp_write_tree.git_cmd = 1;
- argv_array_push(_write_tree.args, "write-tree");
- argv_array_pushf(_write_tree.env_array, "GIT_INDEX_FILE=%s",
-stash_index_path.buf);
- if (pipe_command(_write_tree, NULL, 0, , 0,NULL, 0)) {
+ if (write_index_as_tree(>w_tree, , stash_index_path.buf, 0,
+ NULL)) {
ret = -1;
goto done;
}
- get_oid_hex(out.buf, >w_tree);
-
cp_diff_tree.git_cmd = 1;
argv_array_pushl(_diff_tree.args, "diff-tree", "-p", "HEAD",
oid_to_hex(>w_tree), "--", NULL);
@@ -1011,7 +1000,7 @@ static int stash_patch(struct stash_info *info, struct
pathspec ps, int quiet)
}
done:
- strbuf_release();
+ discard_index();
remove_path(stash_index_path.buf);
return ret;
}
@@ -1020,10 +1009,9 @@ static int stash_working_tree(struct stash_info *info,
struct pathspec ps)
{
int ret = 0;
struct child_process cp_upd_index = CHILD_PROCESS_INIT;
- struct child_process cp_write_tree = CHILD_PROCESS_INIT;
- struct strbuf out = STRBUF_INIT;
struct strbuf diff_output = STRBUF_INIT;
struct rev_info rev;
+ struct index_state istate = { NULL };
set_alternate_index_output(stash_index_path.buf);
if (reset_tree(>i_tree, 0, 0)) {
@@ -1062,20 +1050,15 @@ static int stash_working_tree(struct stash_info *info,
struct pathspec ps)
goto done;
}
- cp_write_tree.git_cmd = 1;
- argv_array_push(_write_tree.args, "write-tree");
- argv_array_pushf(_write_tree.env_array, "GIT_INDEX_FILE=%s",
-stash_index_path.buf);
- if (pipe_command(_write_tree, NULL, 0, , 0,NULL, 0)) {
+ if (write_index_as_tree(>w_tree, , stash_index_path.buf, 0,
+ NULL)) {
ret = -1;
goto done;
}
- get_oid_hex(out.buf, >w_tree);
-
done:
+ discard_index();
UNLEAK(rev);
- strbuf_release();
object_array_clear();
strbuf_release(_output);
remove_path(stash_index_path.buf);
--
2.19.0.rc0.22.gc26283d74e
[GSoC][PATCH v8 19/20] stash: optimize `get_untracked_files()` and `check_changes()`
This commits introduces a optimization by avoiding calling the
same functions again. For example, `git stash push -u`
would call at some points the following functions:
* `check_changes()` (inside `do_push_stash()`)
* `do_create_stash()`, which calls: `check_changes()` and
`get_untracked_files()`
Note that `check_changes()` also calls `get_untracked_files()`.
So, `check_changes()` is called 2 times and `get_untracked_files()`
3 times.
`get_untracked_files()` has now only two parameters and it will
fill a global strbuf called `untracked_files`.
The old function `check_changes()` now consists of two functions:
`get_untracked_files()` and `check_changes_tracked_files()`.
These are the call chains for `push` and `create`:
* `push_stash()` -> `do_push_stash()` -> `do_create_stash()`
* `create_stash()` -> `do_create_stash()`
To prevent calling the same functions over and over again,
`check_changes()` inside `do_create_stash()` is now placed
in the caller functions (`create_stash()` and `do_push_stash()`).
This way `check_changes()` and `get_untracked files()` are called
only one time.
https://public-inbox.org/git/20180818223329.gj11...@hank.intra.tgummerer.com/
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash.c | 73 ++---
1 file changed, 33 insertions(+), 40 deletions(-)
diff --git a/builtin/stash.c b/builtin/stash.c
index 3d2316e3f7..ba5818e24e 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -813,13 +813,15 @@ static int store_stash(int argc, const char **argv, const
char *prefix)
}
/*
- * `out` will be filled with the names of untracked files. The return value is:
+ * `untracked_files` will be filled with the names of untracked files.
+ * The return value is:
*
* = 0 if there are not any untracked files
* > 0 if there are untracked files
*/
-static int get_untracked_files(struct pathspec ps, int include_untracked,
- struct strbuf *out)
+static struct strbuf untracked_files = STRBUF_INIT;
+
+static int get_untracked_files(struct pathspec ps, int include_untracked)
{
int max_len;
int i;
@@ -839,7 +841,7 @@ static int get_untracked_files(struct pathspec ps, int
include_untracked,
free(ent);
continue;
}
- strbuf_addf(out, "%s%c", ent->name, '\0');
+ strbuf_addf(_files, "%s%c", ent->name, '\0');
free(ent);
}
@@ -847,23 +849,22 @@ static int get_untracked_files(struct pathspec ps, int
include_untracked,
free(dir.ignored);
clear_directory();
free(seen);
- return out->len;
+ return untracked_files.len;
}
/*
- * The return value of `check_changes()` can be:
+ * The return value of `check_changes_tracked_files()` can be:
*
* < 0 if there was an error
* = 0 if there are no changes.
* > 0 if there are changes.
*/
-static int check_changes(struct pathspec ps, int include_untracked)
+
+static int check_changes_tracked_files(struct pathspec ps)
{
int result;
- int ret = 0;
struct rev_info rev;
struct object_id dummy;
- struct strbuf out = STRBUF_INIT;
init_revisions(, NULL);
rev.prune_data = ps;
@@ -890,18 +891,22 @@ static int check_changes(struct pathspec ps, int
include_untracked)
if (diff_result_code(, result))
return 1;
- if (include_untracked && get_untracked_files(ps, include_untracked,
-)) {
- strbuf_release();
- return 1;
- }
-
- strbuf_release();
return 0;
}
-static int save_untracked_files(struct stash_info *info, struct strbuf *msg,
- struct strbuf *in)
+static int check_changes(struct pathspec ps, int include_untracked)
+{
+ int ret = 0;
+ if (check_changes_tracked_files(ps))
+ ret = 1;
+
+ if (include_untracked && get_untracked_files(ps, include_untracked))
+ ret = 1;
+
+ return ret;
+}
+
+static int save_untracked_files(struct stash_info *info, struct strbuf *msg)
{
int ret = 0;
struct strbuf untracked_msg = STRBUF_INIT;
@@ -916,7 +921,8 @@ static int save_untracked_files(struct stash_info *info,
struct strbuf *msg,
stash_index_path.buf);
strbuf_addf(_msg, "untracked files on %s\n", msg->buf);
- if (pipe_command(_upd_index, in->buf, in->len, NULL, 0, NULL, 0)) {
+ if (pipe_command(_upd_index, untracked_files.buf,
untracked_files.len,
+NULL, 0, NULL, 0)) {
ret = -1;
goto done;
}
@@ -1090,18 +1096,11 @@ static int do_create_stash(struct pathspec ps, const
char **stash_msg,
struct commit_list *parents = NULL;
struct strbuf msg = STRBUF_INIT;
struct strbuf commit_tree_label = STRBUF_INIT;
-
[GSoC][PATCH v8 15/20] stash: convert push to builtin
Add stash push to the helper.
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 229
git-stash.sh| 6 +-
2 files changed, 233 insertions(+), 2 deletions(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index ce360a569d..23670321d8 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -21,6 +21,9 @@ static const char * const git_stash_helper_usage[] = {
N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet]
[]"),
N_("git stash--helper branch []"),
N_("git stash--helper clear"),
+ N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index]
[-q|--quiet]\n"
+ " [-u|--include-untracked] [-a|--all] [-m|--message
]\n"
+ " [--] [...]]"),
NULL
};
@@ -69,6 +72,13 @@ static const char * const git_stash_helper_create_usage[] = {
NULL
};
+static const char * const git_stash_helper_push_usage[] = {
+ N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index]
[-q|--quiet]\n"
+ " [-u|--include-untracked] [-a|--all] [-m|--message
]\n"
+ " [--] [...]]"),
+ NULL
+};
+
static const char *ref_stash = "refs/stash";
static int quiet;
static struct strbuf stash_index_path = STRBUF_INIT;
@@ -1204,6 +1214,223 @@ static int create_stash(int argc, const char **argv,
const char *prefix)
return ret < 0;
}
+static void add_ps_items_to_argv_array(struct argv_array *args,
+ struct pathspec ps) {
+ int i;
+ for (i = 0; i < ps.nr; ++i)
+ argv_array_push(args, ps.items[i].match);
+}
+
+static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet,
+int keep_index, int patch_mode, int include_untracked)
+{
+ int ret = 0;
+ struct stash_info info;
+ if (patch_mode && keep_index == -1)
+ keep_index = 1;
+
+ if (patch_mode && include_untracked) {
+ fprintf_ln(stderr, _("Can't use --patch and --include-untracked
or --all at the same time"));
+ return -1;
+ }
+
+ read_cache_preload(NULL);
+ if (!include_untracked && ps.nr) {
+ int i;
+ char *ps_matched = xcalloc(ps.nr, 1);
+
+ for (i = 0; i < active_nr; ++i) {
+ const struct cache_entry *ce = active_cache[i];
+ ce_path_match(_index, ce, , ps_matched);
+ }
+
+ if (report_path_error(ps_matched, , NULL)) {
+ fprintf_ln(stderr, _("Did you forget to 'git add'?"));
+ return -1;
+ }
+ free(ps_matched);
+ }
+
+ if (refresh_cache(REFRESH_QUIET))
+ return -1;
+
+ if (!check_changes(ps, include_untracked)) {
+ printf_ln(_("No local changes to save"));
+ return 0;
+ }
+
+ if (!reflog_exists(ref_stash) && do_clear_stash()) {
+ fprintf_ln(stderr, _("Cannot initialize stash"));
+ return -1;
+ }
+
+ if (do_create_stash(ps, _msg, include_untracked, patch_mode,
+ )) {
+ ret = -1;
+ goto done;
+ }
+
+ if (do_store_stash(oid_to_hex(_commit), stash_msg, 1)) {
+ fprintf(stderr, _("Cannot save the current status"));
+ ret = -1;
+ goto done;
+ }
+
+ printf_ln(_("Saved working directory and index state %s"), stash_msg);
+
+ if (!patch_mode) {
+ if (include_untracked && !ps.nr) {
+ struct child_process cp = CHILD_PROCESS_INIT;
+
+ cp.git_cmd = 1;
+ argv_array_pushl(, "clean", "--force",
+"--quiet", "-d", NULL);
+ if (include_untracked == 2)
+ argv_array_push(, "-x");
+ if (run_command()) {
+ ret = -1;
+ goto done;
+ }
+ }
+ if (ps.nr) {
+ struct child_process cp1 = CHILD_PROCESS_INIT;
+ struct child_process cp2 = CHILD_PROCESS_INIT;
+ struct child_process cp3 = CHILD_PROCESS_INIT;
+ struct strbuf out = STRBUF_INIT;
+
+ cp1.git_cmd = 1;
+ argv_array_push(, "add");
+ if (!include_untracked)
+ argv_array_push(, "-u");
+ if (include_untracked == 2)
+ argv_array_push(, "--force");
+ argv_array_push(, "--");
+ add_ps_items_to_argv_array(, ps);
+ if
[GSoC][PATCH v8 07/20] stash: convert drop and clear to builtin
From: Joel Teichroeb
Add the drop and clear commands to the builtin helper. These two
are each simple, but are being added together as they are quite
related.
We have to unfortunately keep the drop and clear functions in the
shell script as functions are called with parameters internally
that are not valid when the commands are called externally. Once
pop is converted they can both be removed.
Signed-off-by: Joel Teichroeb
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 115
git-stash.sh| 4 +-
2 files changed, 117 insertions(+), 2 deletions(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index fde795a764..cbe23fef11 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -12,7 +12,14 @@
#include "rerere.h"
static const char * const git_stash_helper_usage[] = {
+ N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
+ N_("git stash--helper clear"),
+ NULL
+};
+
+static const char * const git_stash_helper_drop_usage[] = {
+ N_("git stash--helper drop [-q|--quiet] []"),
NULL
};
@@ -21,6 +28,11 @@ static const char * const git_stash_helper_apply_usage[] = {
NULL
};
+static const char * const git_stash_helper_clear_usage[] = {
+ N_("git stash--helper clear"),
+ NULL
+};
+
static const char *ref_stash = "refs/stash";
static int quiet;
static struct strbuf stash_index_path = STRBUF_INIT;
@@ -140,6 +152,31 @@ static int get_stash_info(struct stash_info *info, int
argc, const char **argv)
return !(ret == 0 || ret == 1);
}
+static int do_clear_stash(void)
+{
+ struct object_id obj;
+ if (get_oid(ref_stash, ))
+ return 0;
+
+ return delete_ref(NULL, ref_stash, , 0);
+}
+
+static int clear_stash(int argc, const char **argv, const char *prefix)
+{
+ struct option options[] = {
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_clear_usage,
+PARSE_OPT_STOP_AT_NON_OPTION);
+
+ if (argc)
+ return error(_("git stash clear with parameters is
unimplemented"));
+
+ return do_clear_stash();
+}
+
static int reset_tree(struct object_id *i_tree, int update, int reset)
{
struct unpack_trees_options opts;
@@ -424,6 +461,80 @@ static int apply_stash(int argc, const char **argv, const
char *prefix)
return ret;
}
+static int do_drop_stash(const char *prefix, struct stash_info *info)
+{
+ struct child_process cp_reflog = CHILD_PROCESS_INIT;
+ struct child_process cp = CHILD_PROCESS_INIT;
+ int ret;
+
+ /*
+* reflog does not provide a simple function for deleting refs. One will
+* need to be added to avoid implementing too much reflog code here
+*/
+
+ cp_reflog.git_cmd = 1;
+ argv_array_pushl(_reflog.args, "reflog", "delete", "--updateref",
+"--rewrite", NULL);
+ argv_array_push(_reflog.args, info->revision.buf);
+ ret = run_command(_reflog);
+ if (!ret) {
+ if (!quiet)
+ printf_ln(_("Dropped %s (%s)"), info->revision.buf,
+ oid_to_hex(>w_commit));
+ } else {
+ return error(_("%s: Could not drop stash entry"),
+info->revision.buf);
+ }
+
+ /*
+* This could easily be replaced by get_oid, but currently it will throw
+* a fatal error when a reflog is empty, which we can not recover from.
+*/
+ cp.git_cmd = 1;
+ /* Even though --quiet is specified, rev-parse still outputs the hash */
+ cp.no_stdout = 1;
+ argv_array_pushl(, "rev-parse", "--verify", "--quiet", NULL);
+ argv_array_pushf(, "%s@{0}", ref_stash);
+ ret = run_command();
+
+ /* do_clear_stash if we just dropped the last stash entry */
+ if (ret)
+ do_clear_stash();
+
+ return 0;
+}
+
+static void assert_stash_ref(struct stash_info *info)
+{
+ if (!info->is_stash_ref) {
+ free_stash_info(info);
+ error(_("'%s' is not a stash reference"), info->revision.buf);
+ exit(128);
+ }
+}
+
+static int drop_stash(int argc, const char **argv, const char *prefix)
+{
+ struct stash_info info;
+ int ret;
+ struct option options[] = {
+ OPT__QUIET(, N_("be quiet, only report errors")),
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_drop_usage, 0);
+
+ if (get_stash_info(, argc, argv))
+ return -1;
+
+ assert_stash_ref();
+
+ ret = do_drop_stash(prefix, );
+ free_stash_info();
+ return ret;
+}
+
int
[GSoC][PATCH v8 04/20] stash: rename test cases to be more descriptive
Rename some test cases' labels to be more descriptive and under 80
characters per line.
Signed-off-by: Paul-Sebastian Ungureanu
---
t/t3903-stash.sh | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index de6cab1fe7..3114c7bc4c 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -604,7 +604,7 @@ test_expect_success 'stash show -p - no stashes on stack,
stash-like argument' '
test_cmp expected actual
'
-test_expect_success 'stash drop - fail early if specified stash is not a stash
reference' '
+test_expect_success 'drop: fail early if specified stash is not a stash ref' '
git stash clear &&
test_when_finished "git reset --hard HEAD && git stash clear" &&
git reset --hard &&
@@ -618,7 +618,7 @@ test_expect_success 'stash drop - fail early if specified
stash is not a stash r
git reset --hard HEAD
'
-test_expect_success 'stash pop - fail early if specified stash is not a stash
reference' '
+test_expect_success 'pop: fail early if specified stash is not a stash ref' '
git stash clear &&
test_when_finished "git reset --hard HEAD && git stash clear" &&
git reset --hard &&
@@ -682,7 +682,7 @@ test_expect_success 'invalid ref of the form "n", n >= N' '
git stash drop
'
-test_expect_success 'stash branch should not drop the stash if the branch
exists' '
+test_expect_success 'branch: do not drop the stash if the branch exists' '
git stash clear &&
echo foo >file &&
git add file &&
@@ -693,7 +693,7 @@ test_expect_success 'stash branch should not drop the stash
if the branch exists
git rev-parse stash@{0} --
'
-test_expect_success 'stash branch should not drop the stash if the apply
fails' '
+test_expect_success 'branch: should not drop the stash if the apply fails' '
git stash clear &&
git reset HEAD~1 --hard &&
echo foo >file &&
@@ -707,7 +707,7 @@ test_expect_success 'stash branch should not drop the stash
if the apply fails'
git rev-parse stash@{0} --
'
-test_expect_success 'stash apply shows status same as git status (relative to
current directory)' '
+test_expect_success 'apply: show same status as git status (relative to ./)' '
git stash clear &&
echo 1 >subdir/subfile1 &&
echo 2 >subdir/subfile2 &&
@@ -1048,7 +1048,7 @@ test_expect_success 'stash push -p with pathspec shows no
changes only once' '
test_i18ncmp expect actual
'
-test_expect_success 'stash push with pathspec shows no changes when there are
none' '
+test_expect_success 'push : show no changes when there are none' '
>foo &&
git add foo &&
git commit -m "tmp" &&
@@ -1058,7 +1058,7 @@ test_expect_success 'stash push with pathspec shows no
changes when there are no
test_i18ncmp expect actual
'
-test_expect_success 'stash push with pathspec not in the repository errors
out' '
+test_expect_success 'push: not in the repository errors out' '
>untracked &&
test_must_fail git stash push untracked &&
test_path_is_file untracked
--
2.19.0.rc0.22.gc26283d74e
[GSoC][PATCH v8 08/20] stash: convert branch to builtin
From: Joel Teichroeb
Add stash branch to the helper and delete the apply_to_branch
function from the shell script.
Checkout does not currently provide a function for checking out
a branch as cmd_checkout does a large amount of sanity checks
first that we require here.
Signed-off-by: Joel Teichroeb
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 44 +
git-stash.sh| 17 ++--
2 files changed, 46 insertions(+), 15 deletions(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index cbe23fef11..dadc028649 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -14,6 +14,7 @@
static const char * const git_stash_helper_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
+ N_("git stash--helper branch []"),
N_("git stash--helper clear"),
NULL
};
@@ -28,6 +29,11 @@ static const char * const git_stash_helper_apply_usage[] = {
NULL
};
+static const char * const git_stash_helper_branch_usage[] = {
+ N_("git stash--helper branch []"),
+ NULL
+};
+
static const char * const git_stash_helper_clear_usage[] = {
N_("git stash--helper clear"),
NULL
@@ -535,6 +541,42 @@ static int drop_stash(int argc, const char **argv, const
char *prefix)
return ret;
}
+static int branch_stash(int argc, const char **argv, const char *prefix)
+{
+ const char *branch = NULL;
+ int ret;
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct stash_info info;
+ struct option options[] = {
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_branch_usage, 0);
+
+ if (!argc)
+ return error(_("No branch name specified"));
+
+ branch = argv[0];
+
+ if (get_stash_info(, argc - 1, argv + 1))
+ return -1;
+
+ cp.git_cmd = 1;
+ argv_array_pushl(, "checkout", "-b", NULL);
+ argv_array_push(, branch);
+ argv_array_push(, oid_to_hex(_commit));
+ ret = run_command();
+ if (!ret)
+ ret = do_apply_stash(prefix, , 1);
+ if (!ret && info.is_stash_ref)
+ ret = do_drop_stash(prefix, );
+
+ free_stash_info();
+
+ return ret;
+}
+
int cmd_stash__helper(int argc, const char **argv, const char *prefix)
{
pid_t pid = getpid();
@@ -561,6 +603,8 @@ int cmd_stash__helper(int argc, const char **argv, const
char *prefix)
return !!clear_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "drop"))
return !!drop_stash(argc, argv, prefix);
+ else if (!strcmp(argv[0], "branch"))
+ return !!branch_stash(argc, argv, prefix);
usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index a99d5dc9e5..29d9f44255 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -598,20 +598,6 @@ drop_stash () {
clear_stash
}
-apply_to_branch () {
- test -n "$1" || die "$(gettext "No branch name specified")"
- branch=$1
- shift 1
-
- set -- --index "$@"
- assert_stash_like "$@"
-
- git checkout -b $branch $REV^ &&
- apply_stash "$@" && {
- test -z "$IS_STASH_REF" || drop_stash "$@"
- }
-}
-
test "$1" = "-p" && set "push" "$@"
PARSE_CACHE='--not-parsed'
@@ -673,7 +659,8 @@ pop)
;;
branch)
shift
- apply_to_branch "$@"
+ cd "$START_DIR"
+ git stash--helper branch "$@"
;;
*)
case $# in
--
2.19.0.rc0.22.gc26283d74e
[GSoC][PATCH v8 05/20] stash: add tests for `git stash show` config
This commit introduces tests for `git stash show`
config. It tests all the cases where `stash.showStat`
and `stash.showPatch` are unset or set to true / false.
Signed-off-by: Paul-Sebastian Ungureanu
---
t/t3907-stash-show-config.sh | 81
1 file changed, 81 insertions(+)
create mode 100755 t/t3907-stash-show-config.sh
diff --git a/t/t3907-stash-show-config.sh b/t/t3907-stash-show-config.sh
new file mode 100755
index 00..8fe369c1a1
--- /dev/null
+++ b/t/t3907-stash-show-config.sh
@@ -0,0 +1,81 @@
+#!/bin/sh
+
+test_description='Test git stash show configuration.'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ test_commit file
+'
+
+# takes three parameters:
+# 1. the stash.showStat value (or "")
+# 2. the stash.showPatch value (or "")
+# 3. the diff options of the expected output (or nothing for no output)
+test_stat_and_patch () {
+ if test "" = "$1"
+ then
+ test_might_fail git config --unset stash.showStat
+ else
+ test_config stash.showStat "$1"
+ fi &&
+
+ if test "" = "$2"
+ then
+ test_might_fail git config --unset stash.showPatch
+ else
+ test_config stash.showPatch "$2"
+ fi &&
+
+ shift &&
+ shift &&
+ echo 2 >file.t &&
+ git diff "$@" >expect &&
+ git stash &&
+ git stash show >actual &&
+
+ if test -z "$1"
+ then
+ test_must_be_empty actual
+ else
+ test_cmp expect actual
+ fi
+}
+
+test_expect_success 'showStat unset showPatch unset' '
+ test_stat_and_patch "" "" --stat
+'
+
+test_expect_success 'showStat unset showPatch false' '
+ test_stat_and_patch "" false --stat
+'
+
+test_expect_success 'showStat unset showPatch true' '
+ test_stat_and_patch "" true --stat -p
+'
+
+test_expect_success 'showStat false showPatch unset' '
+ test_stat_and_patch false ""
+'
+
+test_expect_success 'showStat false showPatch false' '
+ test_stat_and_patch false false
+'
+
+test_expect_success 'showStat false showPatch true' '
+ test_stat_and_patch false true -p
+'
+
+test_expect_success 'showStat true showPatch unset' '
+ test_stat_and_patch true "" --stat
+'
+
+test_expect_success 'showStat true showPatch false' '
+ test_stat_and_patch true false --stat
+'
+
+test_expect_success 'showStat true showPatch true' '
+ test_stat_and_patch true true --stat -p
+'
+
+test_done
--
2.19.0.rc0.22.gc26283d74e
[GSoC][PATCH v8 03/20] stash: update test cases conform to coding guidelines
Removed whitespaces after redirection operators.
Signed-off-by: Paul-Sebastian Ungureanu
---
t/t3903-stash.sh | 120 ---
1 file changed, 61 insertions(+), 59 deletions(-)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index af7586d43d..de6cab1fe7 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -8,22 +8,22 @@ test_description='Test git stash'
. ./test-lib.sh
test_expect_success 'stash some dirty working directory' '
- echo 1 > file &&
+ echo 1 >file &&
git add file &&
echo unrelated >other-file &&
git add other-file &&
test_tick &&
git commit -m initial &&
- echo 2 > file &&
+ echo 2 >file &&
git add file &&
- echo 3 > file &&
+ echo 3 >file &&
test_tick &&
git stash &&
git diff-files --quiet &&
git diff-index --cached --quiet HEAD
'
-cat > expect << EOF
+cat >expect < output &&
+ git diff stash^2..stash >output &&
test_cmp output expect
'
@@ -74,7 +74,7 @@ test_expect_success 'apply stashed changes' '
test_expect_success 'apply stashed changes (including index)' '
git reset --hard HEAD^ &&
- echo 6 > other-file &&
+ echo 6 >other-file &&
git add other-file &&
test_tick &&
git commit -m other-file &&
@@ -99,12 +99,12 @@ test_expect_success 'stash drop complains of extra options'
'
test_expect_success 'drop top stash' '
git reset --hard &&
- git stash list > stashlist1 &&
- echo 7 > file &&
+ git stash list >expected &&
+ echo 7 >file &&
git stash &&
git stash drop &&
- git stash list > stashlist2 &&
- test_cmp stashlist1 stashlist2 &&
+ git stash list >actual &&
+ test_cmp expected actual &&
git stash apply &&
test 3 = $(cat file) &&
test 1 = $(git show :file) &&
@@ -113,9 +113,9 @@ test_expect_success 'drop top stash' '
test_expect_success 'drop middle stash' '
git reset --hard &&
- echo 8 > file &&
+ echo 8 >file &&
git stash &&
- echo 9 > file &&
+ echo 9 >file &&
git stash &&
git stash drop stash@{1} &&
test 2 = $(git stash list | wc -l) &&
@@ -160,7 +160,7 @@ test_expect_success 'stash pop' '
test 0 = $(git stash list | wc -l)
'
-cat > expect << EOF
+cat >expect < expect1 << EOF
+cat >expect1 < expect2 << EOF
+cat >expect2 < file &&
+ echo foo >file &&
git commit file -m first &&
- echo bar > file &&
- echo bar2 > file2 &&
+ echo bar >file &&
+ echo bar2 >file2 &&
git add file2 &&
git stash &&
- echo baz > file &&
+ echo baz >file &&
git commit file -m second &&
git stash branch stashbranch &&
test refs/heads/stashbranch = $(git symbolic-ref HEAD) &&
test $(git rev-parse HEAD) = $(git rev-parse master^) &&
- git diff --cached > output &&
+ git diff --cached >output &&
test_cmp output expect &&
- git diff > output &&
+ git diff >output &&
test_cmp output expect1 &&
git add file &&
git commit -m alternate\ second &&
- git diff master..stashbranch > output &&
+ git diff master..stashbranch >output &&
test_cmp output expect2 &&
test 0 = $(git stash list | wc -l)
'
test_expect_success 'apply -q is quiet' '
- echo foo > file &&
+ echo foo >file &&
git stash &&
- git stash apply -q > output.out 2>&1 &&
+ git stash apply -q >output.out 2>&1 &&
test_must_be_empty output.out
'
test_expect_success 'save -q is quiet' '
- git stash save --quiet > output.out 2>&1 &&
+ git stash save --quiet >output.out 2>&1 &&
test_must_be_empty output.out
'
test_expect_success 'pop -q is quiet' '
- git stash pop -q > output.out 2>&1 &&
+ git stash pop -q >output.out 2>&1 &&
test_must_be_empty output.out
'
test_expect_success 'pop -q --index works and is quiet' '
- echo foo > file &&
+ echo foo >file &&
git add file &&
git stash save --quiet &&
- git stash pop -q --index > output.out 2>&1 &&
+ git stash pop -q --index >output.out 2>&1 &&
test foo = "$(git show :file)" &&
test_must_be_empty output.out
'
test_expect_success 'drop -q is quiet' '
git stash &&
- git stash drop -q > output.out 2>&1 &&
+ git stash drop -q >output.out 2>&1 &&
test_must_be_empty output.out
'
test_expect_success 'stash -k' '
- echo bar3 > file &&
- echo bar4 > file2 &&
+ echo bar3 >file &&
+ echo bar4 >file2 &&
git add file2 &&
git stash -k &&
test bar,bar4 = $(cat file),$(cat file2)
'
test_expect_success 'stash --no-keep-index' '
- echo bar33 > file &&
- echo bar44 > file2 &&
+ echo bar33 >file &&
+ echo
[GSoC][PATCH v8 02/20] stash: improve option parsing test coverage
From: Joel Teichroeb
In preparation for converting the stash command incrementally to
a builtin command, this patch improves test coverage of the option
parsing. Both for having too many parameters, or too few.
Signed-off-by: Joel Teichroeb
Signed-off-by: Paul-Sebastian Ungureanu
---
t/t3903-stash.sh | 35 +++
1 file changed, 35 insertions(+)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 1f871d3cca..af7586d43d 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -444,6 +444,36 @@ test_expect_failure 'stash file to directory' '
test foo = "$(cat file/file)"
'
+test_expect_success 'giving too many ref arguments does not modify files' '
+ git stash clear &&
+ test_when_finished "git reset --hard HEAD" &&
+ echo foo >file2 &&
+ git stash &&
+ echo bar >file2 &&
+ git stash &&
+ test-tool chmtime =123456789 file2 &&
+ for type in apply pop "branch stash-branch"
+ do
+ test_must_fail git stash $type stash@{0} stash@{1} 2>err &&
+ test_i18ngrep "Too many revisions" err &&
+ test 123456789 = $(test-tool chmtime -g file2) || return 1
+ done
+'
+
+test_expect_success 'drop: too many arguments errors out (does nothing)' '
+ git stash list >expect &&
+ test_must_fail git stash drop stash@{0} stash@{1} 2>err &&
+ test_i18ngrep "Too many revisions" err &&
+ git stash list >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'show: too many arguments errors out (does nothing)' '
+ test_must_fail git stash show stash@{0} stash@{1} 2>err 1>out &&
+ test_i18ngrep "Too many revisions" err &&
+ test_must_be_empty out
+'
+
test_expect_success 'stash create - no changes' '
git stash clear &&
test_when_finished "git reset --hard HEAD" &&
@@ -479,6 +509,11 @@ test_expect_success 'stash branch - stashes on stack,
stash-like argument' '
test $(git ls-files --modified | wc -l) -eq 1
'
+test_expect_success 'stash branch complains with no arguments' '
+ test_must_fail git stash branch 2>err &&
+ test_i18ngrep "No branch name specified" err
+'
+
test_expect_success 'stash show format defaults to --stat' '
git stash clear &&
test_when_finished "git reset --hard HEAD" &&
--
2.19.0.rc0.22.gc26283d74e
[GSoC][PATCH v8 00/20] Convert "git stash" to C builtin
Hello, This a new iteration of `stash.c`. What is new? * Some commits got squashed. The commit related to replacing `git apply` child process was dropped since it wasn't the best idea. * In v7, there was a bug [1] related to config `git stash show` The bug was fixed and a test file was added for this. * Fixed `git stash [push]` [2]. In v7, `git stash -q drop` would act like `git stash push -q drop`. * Fixed coding-style nits. Verified that messages are marked for translation and are going to the correct output stream. * Fixed one memory leak (related to `strbuf_detach`). * Simplified the code a little bit. [1]: https://public-inbox.org/git/20180815210142.gn2...@hank.intra.tgummerer.com/ [2]: https://public-inbox.org/git/20180818165102.gf11...@hank.intra.tgummerer.com/ Best regards, Paul Joel Teichroeb (5): stash: improve option parsing test coverage stash: convert apply to builtin stash: convert drop and clear to builtin stash: convert branch to builtin stash: convert pop to builtin Paul-Sebastian Ungureanu (15): sha1-name.c: add `get_oidf()` which acts like `get_oid()` stash: update test cases conform to coding guidelines stash: rename test cases to be more descriptive stash: add tests for `git stash show` config stash: convert list to builtin stash: convert show to builtin stash: mention options in `show` synopsis. stash: convert store to builtin stash: convert create to builtin stash: convert push to builtin stash: make push -q quiet stash: convert save to builtin stash: convert `stash--helper.c` into `stash.c` stash: optimize `get_untracked_files()` and `check_changes()` stash: replace all `write-tree` child processes with API calls Documentation/git-stash.txt |4 +- Makefile |2 +- builtin.h|1 + builtin/stash.c | 1563 ++ cache.h |1 + git-stash.sh | 752 git.c|1 + sha1-name.c | 19 + t/t3903-stash.sh | 192 +++-- t/t3907-stash-show-config.sh | 81 ++ 10 files changed, 1795 insertions(+), 821 deletions(-) create mode 100644 builtin/stash.c delete mode 100755 git-stash.sh create mode 100755 t/t3907-stash-show-config.sh -- 2.19.0.rc0.22.gc26283d74e
[GSoC][PATCH v8 06/20] stash: convert apply to builtin
From: Joel Teichroeb
Add a builtin helper for performing stash commands. Converting
all at once proved hard to review, so starting with just apply
lets conversion get started without the other commands being
finished.
The helper is being implemented as a drop in replacement for
stash so that when it is complete it can simply be renamed and
the shell script deleted.
Delete the contents of the apply_stash shell function and replace
it with a call to stash--helper apply until pop is also
converted.
Signed-off-by: Joel Teichroeb
Signed-off-by: Paul-Sebastian Ungureanu
---
.gitignore | 1 +
Makefile| 1 +
builtin.h | 1 +
builtin/stash--helper.c | 452
git-stash.sh| 78 +--
git.c | 1 +
6 files changed, 463 insertions(+), 71 deletions(-)
create mode 100644 builtin/stash--helper.c
diff --git a/.gitignore b/.gitignore
index ffceea7d59..b59661cb88 100644
--- a/.gitignore
+++ b/.gitignore
@@ -157,6 +157,7 @@
/git-show-ref
/git-stage
/git-stash
+/git-stash--helper
/git-status
/git-stripspace
/git-submodule
diff --git a/Makefile b/Makefile
index d03df31c2a..f900c68e69 100644
--- a/Makefile
+++ b/Makefile
@@ -1093,6 +1093,7 @@ BUILTIN_OBJS += builtin/shortlog.o
BUILTIN_OBJS += builtin/show-branch.o
BUILTIN_OBJS += builtin/show-index.o
BUILTIN_OBJS += builtin/show-ref.o
+BUILTIN_OBJS += builtin/stash--helper.o
BUILTIN_OBJS += builtin/stripspace.o
BUILTIN_OBJS += builtin/submodule--helper.o
BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index 99206df4bd..317bc338f7 100644
--- a/builtin.h
+++ b/builtin.h
@@ -223,6 +223,7 @@ extern int cmd_show(int argc, const char **argv, const char
*prefix);
extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
extern int cmd_show_index(int argc, const char **argv, const char *prefix);
extern int cmd_status(int argc, const char **argv, const char *prefix);
+extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);
extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
extern int cmd_submodule__helper(int argc, const char **argv, const char
*prefix);
extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
new file mode 100644
index 00..fde795a764
--- /dev/null
+++ b/builtin/stash--helper.c
@@ -0,0 +1,452 @@
+#include "builtin.h"
+#include "config.h"
+#include "parse-options.h"
+#include "refs.h"
+#include "lockfile.h"
+#include "cache-tree.h"
+#include "unpack-trees.h"
+#include "merge-recursive.h"
+#include "argv-array.h"
+#include "run-command.h"
+#include "dir.h"
+#include "rerere.h"
+
+static const char * const git_stash_helper_usage[] = {
+ N_("git stash--helper apply [--index] [-q|--quiet] []"),
+ NULL
+};
+
+static const char * const git_stash_helper_apply_usage[] = {
+ N_("git stash--helper apply [--index] [-q|--quiet] []"),
+ NULL
+};
+
+static const char *ref_stash = "refs/stash";
+static int quiet;
+static struct strbuf stash_index_path = STRBUF_INIT;
+
+/*
+ * w_commit is set to the commit containing the working tree
+ * b_commit is set to the base commit
+ * i_commit is set to the commit containing the index tree
+ * u_commit is set to the commit containing the untracked files tree
+ * w_tree is set to the working tree
+ * b_tree is set to the base tree
+ * i_tree is set to the index tree
+ * u_tree is set to the untracked files tree
+ */
+
+struct stash_info {
+ struct object_id w_commit;
+ struct object_id b_commit;
+ struct object_id i_commit;
+ struct object_id u_commit;
+ struct object_id w_tree;
+ struct object_id b_tree;
+ struct object_id i_tree;
+ struct object_id u_tree;
+ struct strbuf revision;
+ int is_stash_ref;
+ int has_u;
+};
+
+static void free_stash_info(struct stash_info *info)
+{
+ strbuf_release(>revision);
+}
+
+static void assert_stash_like(struct stash_info *info, const char *revision)
+{
+ if (get_oidf(>b_commit, "%s^1", revision) ||
+ get_oidf(>w_tree, "%s:", revision) ||
+ get_oidf(>b_tree, "%s^1:", revision) ||
+ get_oidf(>i_tree, "%s^2:", revision)) {
+ free_stash_info(info);
+ error(_("'%s' is not a stash-like commit"), revision);
+ exit(128);
+ }
+}
+
+static int get_stash_info(struct stash_info *info, int argc, const char **argv)
+{
+ struct strbuf symbolic = STRBUF_INIT;
+ int ret;
+ const char *revision;
+ const char *commit = NULL;
+ char *end_of_rev;
+ char *expanded_ref;
+ struct object_id dummy;
+
+ if (argc > 1) {
+ int i;
+ struct strbuf refs_msg = STRBUF_INIT;
+ for (i = 0; i < argc; ++i)
+ strbuf_addf(_msg, "
[GSoC][PATCH v8 01/20] sha1-name.c: add `get_oidf()` which acts like `get_oid()`
Compared to `get_oid()`, `get_oidf()` has as parameters
a pointer to `object_id`, a printf format string and
additional arguments. This will help simplify the code
in subsequent commits.
Original-idea-by: Johannes Schindelin
Signed-off-by: Paul-Sebastian Ungureanu
---
cache.h | 1 +
sha1-name.c | 19 +++
2 files changed, 20 insertions(+)
diff --git a/cache.h b/cache.h
index b1fd3d58ab..d93b2e25a5 100644
--- a/cache.h
+++ b/cache.h
@@ -1309,6 +1309,7 @@ struct object_context {
GET_OID_BLOB)
extern int get_oid(const char *str, struct object_id *oid);
+extern int get_oidf(struct object_id *oid, const char *fmt, ...);
extern int get_oid_commit(const char *str, struct object_id *oid);
extern int get_oid_committish(const char *str, struct object_id *oid);
extern int get_oid_tree(const char *str, struct object_id *oid);
diff --git a/sha1-name.c b/sha1-name.c
index c9cc1318b7..261b960bbd 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -1471,6 +1471,25 @@ int get_oid(const char *name, struct object_id *oid)
return get_oid_with_context(name, 0, oid, );
}
+/*
+ * This returns a non-zero value if the string (built using printf
+ * format and the given arguments) is not a valid object.
+ */
+int get_oidf(struct object_id *oid, const char *fmt, ...)
+{
+ va_list ap;
+ int ret;
+ struct strbuf sb = STRBUF_INIT;
+
+ va_start(ap, fmt);
+ strbuf_vaddf(, fmt, ap);
+ va_end(ap);
+
+ ret = get_oid(sb.buf, oid);
+ strbuf_release();
+
+ return ret;
+}
/*
* Many callers know that the user meant to name a commit-ish by
--
2.19.0.rc0.22.gc26283d74e
[GSoC][PATCH v8 09/20] stash: convert pop to builtin
From: Joel Teichroeb
Add stash pop to the helper and delete the pop_stash, drop_stash,
assert_stash_ref functions from the shell script now that they
are no longer needed.
Signed-off-by: Joel Teichroeb
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 36 ++-
git-stash.sh| 47 ++---
2 files changed, 37 insertions(+), 46 deletions(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index dadc028649..9fb1003dbb 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -13,7 +13,7 @@
static const char * const git_stash_helper_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
- N_("git stash--helper apply [--index] [-q|--quiet] []"),
+ N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet]
[]"),
N_("git stash--helper branch []"),
N_("git stash--helper clear"),
NULL
@@ -24,6 +24,11 @@ static const char * const git_stash_helper_drop_usage[] = {
NULL
};
+static const char * const git_stash_helper_pop_usage[] = {
+ N_("git stash--helper pop [--index] [-q|--quiet] []"),
+ NULL
+};
+
static const char * const git_stash_helper_apply_usage[] = {
N_("git stash--helper apply [--index] [-q|--quiet] []"),
NULL
@@ -541,6 +546,33 @@ static int drop_stash(int argc, const char **argv, const
char *prefix)
return ret;
}
+static int pop_stash(int argc, const char **argv, const char *prefix)
+{
+ int index = 0, ret;
+ struct stash_info info;
+ struct option options[] = {
+ OPT__QUIET(, N_("be quiet, only report errors")),
+ OPT_BOOL(0, "index", ,
+ N_("attempt to recreate the index")),
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_pop_usage, 0);
+
+ if (get_stash_info(, argc, argv))
+ return -1;
+
+ assert_stash_ref();
+ if ((ret = do_apply_stash(prefix, , index)))
+ printf_ln(_("The stash entry is kept in case you need it
again."));
+ else
+ ret = do_drop_stash(prefix, );
+
+ free_stash_info();
+ return ret;
+}
+
static int branch_stash(int argc, const char **argv, const char *prefix)
{
const char *branch = NULL;
@@ -603,6 +635,8 @@ int cmd_stash__helper(int argc, const char **argv, const
char *prefix)
return !!clear_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "drop"))
return !!drop_stash(argc, argv, prefix);
+ else if (!strcmp(argv[0], "pop"))
+ return !!pop_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "branch"))
return !!branch_stash(argc, argv, prefix);
diff --git a/git-stash.sh b/git-stash.sh
index 29d9f44255..8f2640fe90 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -554,50 +554,6 @@ assert_stash_like() {
}
}
-is_stash_ref() {
- is_stash_like "$@" && test -n "$IS_STASH_REF"
-}
-
-assert_stash_ref() {
- is_stash_ref "$@" || {
- args="$*"
- die "$(eval_gettext "'\$args' is not a stash reference")"
- }
-}
-
-apply_stash () {
- cd "$START_DIR"
- git stash--helper apply "$@"
- res=$?
- cd_to_toplevel
- return $res
-}
-
-pop_stash() {
- assert_stash_ref "$@"
-
- if apply_stash "$@"
- then
- drop_stash "$@"
- else
- status=$?
- say "$(gettext "The stash entry is kept in case you need it
again.")"
- exit $status
- fi
-}
-
-drop_stash () {
- assert_stash_ref "$@"
-
- git reflog delete --updateref --rewrite "${REV}" &&
- say "$(eval_gettext "Dropped \${REV} (\$s)")" ||
- die "$(eval_gettext "\${REV}: Could not drop stash entry")"
-
- # clear_stash if we just dropped the last stash entry
- git rev-parse --verify --quiet "$ref_stash@{0}" >/dev/null ||
- clear_stash
-}
-
test "$1" = "-p" && set "push" "$@"
PARSE_CACHE='--not-parsed'
@@ -655,7 +611,8 @@ drop)
;;
pop)
shift
- pop_stash "$@"
+ cd "$START_DIR"
+ git stash--helper pop "$@"
;;
branch)
shift
--
2.19.0.rc0.22.gc26283d74e
[GSoC][PATCH v8 11/20] stash: convert show to builtin
Add stash show to the helper and delete the show_stash, have_stash,
assert_stash_like, is_stash_like and parse_flags_and_rev functions
from the shell script now that they are no longer needed.
In shell version, although `git stash show` accepts `--index` and
`--quiet` options, it ignores them. In C, both options are passed
further to `git diff`.
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 87 ++
git-stash.sh| 132 +---
2 files changed, 88 insertions(+), 131 deletions(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index c42a297078..1dba7e6853 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -10,9 +10,12 @@
#include "run-command.h"
#include "dir.h"
#include "rerere.h"
+#include "revision.h"
+#include "log-tree.h"
static const char * const git_stash_helper_usage[] = {
N_("git stash--helper list []"),
+ N_("git stash--helper show []"),
N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet]
[]"),
N_("git stash--helper branch []"),
@@ -25,6 +28,11 @@ static const char * const git_stash_helper_list_usage[] = {
NULL
};
+static const char * const git_stash_helper_show_usage[] = {
+ N_("git stash--helper show []"),
+ NULL
+};
+
static const char * const git_stash_helper_drop_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
NULL
@@ -638,6 +646,83 @@ static int list_stash(int argc, const char **argv, const
char *prefix)
return run_command();
}
+static int show_stat = 1;
+static int show_patch;
+
+static int git_stash_config(const char *var, const char *value, void *cb)
+{
+ if (!strcmp(var, "stash.showstat")) {
+ show_stat = git_config_bool(var, value);
+ return 0;
+ }
+ if (!strcmp(var, "stash.showpatch")) {
+ show_patch = git_config_bool(var, value);
+ return 0;
+ }
+ return git_default_config(var, value, cb);
+}
+
+static int show_stash(int argc, const char **argv, const char *prefix)
+{
+ int i;
+ int opts = 0;
+ int ret = 0;
+ struct stash_info info;
+ struct rev_info rev;
+ struct argv_array stash_args = ARGV_ARRAY_INIT;
+ struct option options[] = {
+ OPT_END()
+ };
+
+ init_diff_ui_defaults();
+ git_config(git_diff_ui_config, NULL);
+ init_revisions(, prefix);
+
+ for (i = 1; i < argc; ++i) {
+ if (argv[i][0] != '-')
+ argv_array_push(_args, argv[i]);
+ else
+ opts++;
+ }
+
+ ret = get_stash_info(, stash_args.argc, stash_args.argv);
+ argv_array_clear(_args);
+ if (ret)
+ return -1;
+
+ /*
+* The config settings are applied only if there are not passed
+* any options.
+*/
+ if (!opts) {
+ git_config(git_stash_config, NULL);
+ if (show_stat)
+ rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT;
+
+ if (show_patch)
+ rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
+
+ if (!show_stat && !show_patch) {
+ free_stash_info();
+ return 0;
+ }
+ }
+
+ argc = setup_revisions(argc, argv, , NULL);
+ if (argc > 1) {
+ free_stash_info();
+ usage_with_options(git_stash_helper_show_usage, options);
+ }
+
+ rev.diffopt.flags.recursive = 1;
+ setup_diff_pager();
+ diff_tree_oid(_commit, _commit, "", );
+ log_tree_diff_flush();
+
+ free_stash_info();
+ return diff_result_code(, 0);
+}
+
int cmd_stash__helper(int argc, const char **argv, const char *prefix)
{
pid_t pid = getpid();
@@ -670,6 +755,8 @@ int cmd_stash__helper(int argc, const char **argv, const
char *prefix)
return !!branch_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "list"))
return !!list_stash(argc, argv, prefix);
+ else if (!strcmp(argv[0], "show"))
+ return !!show_stash(argc, argv, prefix);
usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 6052441aa2..0d05cbc1e5 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -378,35 +378,6 @@ save_stash () {
fi
}
-have_stash () {
- git rev-parse --verify --quiet $ref_stash >/dev/null
-}
-
-show_stash () {
- ALLOW_UNKNOWN_FLAGS=t
- assert_stash_like "$@"
-
- if test -z "$FLAGS"
- then
- if test "$(git config --bool stash.showStat || echo true)" =
"true"
- then
- FLAGS=--stat
-
[GSoC][PATCH v8 10/20] stash: convert list to builtin
Add stash list to the helper and delete the list_stash function
from the shell script.
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 31 +++
git-stash.sh| 7 +--
2 files changed, 32 insertions(+), 6 deletions(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 9fb1003dbb..c42a297078 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -12,6 +12,7 @@
#include "rerere.h"
static const char * const git_stash_helper_usage[] = {
+ N_("git stash--helper list []"),
N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet]
[]"),
N_("git stash--helper branch []"),
@@ -19,6 +20,11 @@ static const char * const git_stash_helper_usage[] = {
NULL
};
+static const char * const git_stash_helper_list_usage[] = {
+ N_("git stash--helper list []"),
+ NULL
+};
+
static const char * const git_stash_helper_drop_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
NULL
@@ -609,6 +615,29 @@ static int branch_stash(int argc, const char **argv, const
char *prefix)
return ret;
}
+static int list_stash(int argc, const char **argv, const char *prefix)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct option options[] = {
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_list_usage,
+PARSE_OPT_KEEP_UNKNOWN);
+
+ if (!ref_exists(ref_stash))
+ return 0;
+
+ cp.git_cmd = 1;
+ argv_array_pushl(, "log", "--format=%gd: %gs", "-g",
+"--first-parent", "-m", NULL);
+ argv_array_pushv(, argv);
+ argv_array_push(, ref_stash);
+ argv_array_push(, "--");
+ return run_command();
+}
+
int cmd_stash__helper(int argc, const char **argv, const char *prefix)
{
pid_t pid = getpid();
@@ -639,6 +668,8 @@ int cmd_stash__helper(int argc, const char **argv, const
char *prefix)
return !!pop_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "branch"))
return !!branch_stash(argc, argv, prefix);
+ else if (!strcmp(argv[0], "list"))
+ return !!list_stash(argc, argv, prefix);
usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 8f2640fe90..6052441aa2 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -382,11 +382,6 @@ have_stash () {
git rev-parse --verify --quiet $ref_stash >/dev/null
}
-list_stash () {
- have_stash || return 0
- git log --format="%gd: %gs" -g --first-parent -m "$@" $ref_stash --
-}
-
show_stash () {
ALLOW_UNKNOWN_FLAGS=t
assert_stash_like "$@"
@@ -574,7 +569,7 @@ test -n "$seen_non_option" || set "push" "$@"
case "$1" in
list)
shift
- list_stash "$@"
+ git stash--helper list "$@"
;;
show)
shift
--
2.19.0.rc0.22.gc26283d74e
[GSoC][PATCH v8 12/20] stash: mention options in `show` synopsis.
Mention in the usage text and in the documentation, that `show`
accepts any option known to `git diff`.
Signed-off-by: Paul-Sebastian Ungureanu
---
Documentation/git-stash.txt | 4 ++--
builtin/stash--helper.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 7ef8c47911..e31ea7d303 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -9,7 +9,7 @@ SYNOPSIS
[verse]
'git stash' list []
-'git stash' show []
+'git stash' show [] []
'git stash' drop [-q|--quiet] []
'git stash' ( pop | apply ) [--index] [-q|--quiet] []
'git stash' branch []
@@ -106,7 +106,7 @@ stash@{1}: On master: 9cc0589... Add git-stash
The command takes options applicable to the 'git log'
command to control what is shown and how. See linkgit:git-log[1].
-show []::
+show [] []::
Show the changes recorded in the stash entry as a diff between the
stashed contents and the commit back when the stash entry was first
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 1dba7e6853..02b593e0cd 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -15,7 +15,7 @@
static const char * const git_stash_helper_usage[] = {
N_("git stash--helper list []"),
- N_("git stash--helper show []"),
+ N_("git stash--helper show [] []"),
N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet]
[]"),
N_("git stash--helper branch []"),
@@ -29,7 +29,7 @@ static const char * const git_stash_helper_list_usage[] = {
};
static const char * const git_stash_helper_show_usage[] = {
- N_("git stash--helper show []"),
+ N_("git stash--helper show [] []"),
NULL
};
--
2.19.0.rc0.22.gc26283d74e
Re: [PATCH v4 4/6] push doc: correct lies about how push refspecs work
Ævar Arnfjörð Bjarmason writes:
> +on the remote side. Whether this is allowed depends on where in
> +`refs/*` the reference lives as described in detail below. Any
> +such update does *not* attempt to merge into . See EXAMPLES
> +below for details.
> ++
> +The `refs/heads/*` namespace will only accept commit objects, and only
> +if they can be fast-forwarded.
> ++
> +The `refs/tags/*` namespace will accept any kind of object (as
> +commits, trees and blobs can be tagged), and any changes to them will
> +be rejected.
> ++
> +It's possible to push any type of object to any namespace outside of
> +`refs/{tags,heads}/*`. In the case of tags and commits, these will be
> +treated as if they were the commits inside `refs/heads/*` for the
> +purposes of whether the update is allowed.
> ++
> +I.e. a fast-forward of commits and tags outside `refs/{tags,heads}/*`
> +is allowed, even in cases where what's being fast-forwarded is not a
> +commit, but a tag object which happens to point to a new commit which
> +is a fast-forward of the commit the last tag (or commit) it's
> +replacing. Replacing a tag with an entirely different tag is also
> +allowed, if it points to the same commit, as well as pushing a peeled
> +tag, i.e. pushing the commit that existing tag object points to, or a
> +new tag object which an existing commit points to.
> ++
> +Tree and blob objects outside of `refs/{tags,heads}/*` will be treated
> +the same way as if they were inside `refs/tags/*`, any modification of
> +them will be rejected.
> ++
> +All of the rules described above about what's not allowed as an update
> +can be overridden by adding an the optional leading `+` to a refspec
> +(or using `--force` command line option). The only exception to this
> +is that no amount of forcing will make the `refs/heads/*` namespace
> +accept a non-commit object.
This, while some may find it overly long, is quite clear, compared
to the current text and to the previous rounds of this patch, and I
found it very much readable.
Re: [PATCH v4 3/6] fetch tests: add a test for clobbering tag behavior
Ævar Arnfjörð Bjarmason writes:
> The test suite only incidentally (and unintentionally) tested for the
> current behavior of eager tag clobbering on "fetch". This is a
> followup to 380efb65df ("push tests: assert re-pushing annotated
> tags", 2018-07-31) which tests for it explicitly.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
Good addition, and correctly uses $tag_type_description unlike the
one that is left unfixed in 2/6 for the push side.
> t/t5516-fetch-push.sh | 24
> 1 file changed, 24 insertions(+)
>
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 69f7c9bfe6..3cde72ae47 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1011,6 +1011,30 @@ test_force_push_tag () {
> test_force_push_tag "lightweight tag" "-f"
> test_force_push_tag "annotated tag" "-f -a -mtag.message"
>
> +test_force_fetch_tag () {
> + tag_type_description=$1
> + tag_args=$2
> +
> + test_expect_success "fetch will clobber an existing
> $tag_type_description" "
> + mk_test testrepo heads/master &&
> + mk_child testrepo child1 &&
> + mk_child testrepo child2 &&
> + (
> + cd testrepo &&
> + git tag testTag &&
> + git -C ../child1 fetch origin tag testTag &&
> + >file1 &&
> + git add file1 &&
> + git commit -m 'file1' &&
> + git tag $tag_args testTag &&
> + git -C ../child1 fetch origin tag testTag
> + )
> + "
> +}
> +
> +test_force_fetch_tag "lightweight tag" "-f"
> +test_force_fetch_tag "annotated tag" "-f -a -mtag.message"
> +
> test_expect_success 'push --porcelain' '
> mk_empty testrepo &&
> echo >.git/foo "To testrepo" &&
Re: [PATCH v4 2/6] push tests: correct quoting in interpolated string
Ævar Arnfjörð Bjarmason writes:
> The quoted -m'msg' option is passed as a string to another function,
> where due to interpolation it'll end up meaning the same as if we did
> just did -m'msg' here.
"as if we did just did"? Also the sentence says -m'msg' is treated
as if we gave -m'msg' that is tautology. Perhaps
... as if we just did -mmsg here.
is what you meant?
But I think the pointing out I did in the old thread is wrong. If
you change your "-mtag.message" to "-m'tag message'" (notice that
I have two spaces between the words) and then insert an invocation
of "git show -s testTag" immediately after "git tag" is run to
create testTag with $tag_args in test_force_push_tag, I can
observe in "cd t && sh t5516-fetch-push.sh -v" output that
the single quote is taking effect just fine. IOW, I do not see
anything wrong in the original "-m'msg'", which probably anticipated
that we may want to change it to -m'tag message' or something to
clarify.
However,...
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 539c25aada..69f7c9bfe6 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1009,7 +1009,7 @@ test_force_push_tag () {
> }
>
> test_force_push_tag "lightweight tag" "-f"
> -test_force_push_tag "annotated tag" "-f -a -m'msg'"
> +test_force_push_tag "annotated tag" "-f -a -mtag.message"
Comparing test_force_push_tag and test_force_fetch_tag which is
added in the next step, I notice that the former ignores $1 so
passing "annotated tag" here has no effect. That may be worth
fixing in a follow-up patch like this.
Re: [PATCH v2 4/4] branch: make "-l" a synonym for "--list"
On Thu, Aug 30 2018, Jeff King wrote:
> On Thu, Aug 30, 2018 at 01:29:53PM -0700, Junio C Hamano wrote:
>
>> >> > I do not know if the documentation that is shipped in 2.20 should
>> >> > talk about how the old world looked like, though. `-l` was a short
>> >> > for `--create-reflog` is worth saying, but I do not see much value
>> >> > in talking about the warning given in 2.19.
>> >>
>> >> I'm anticipating that there will be users in the wild with similar -l
>> >> invocations, noting this helps them, because they'll be wondering what
>> >> some script that does "git branch -l " is trying to do while
>> >> reading our docs.
>> >
>> > I don't have a strong opinion either way. If we do mention it, it should
>> > probably be short ("Until Git v2.20, the `-l` option was a synonym for
>> > `--create-reflog").
>>
>> I agree that the short one would of course be good. I am on the
>> fence about mentioning the warning only given in 2.19.
>
> Yeah, I was confused about that part of the thread. Is there something
> proposed to (additionally) go into v2.19? Ævar, can you elaborate?
The patch I proposed was badly worded and on reflection I don't think
it's useful to include this, but FWIW what I meant was:
* 1. <2.19: -l is --create-reflog
* 2. =2.19: -l is --create-reflog, but will spew a warning to stderr about
futre deprecation
* 3. >2.19: -l is --list
I.e. should we in >2.19 docs say that -l used to mean something
different <= 2.19? Yeah, but it's probably worthless information to say
that it used to warn in that one release, since the actionable thing to
do with this information is to change it to --create-reflog, and unlike
going from >2.19 to <2.19 running =2.19 isn't silently going to treat
the -l option in a way you might not expect.
Re: [PATCH] doc/Makefile: remove tmp-doc-diff on "make clean"
On Thu, Aug 30, 2018 at 04:34:43PM -0400, Eric Sunshine wrote: > On Thu, Aug 30, 2018 at 3:55 PM Jeff King wrote: > > The tmp-doc-diff directory isn't strictly a build product of > > the Makefile, since it's only present if you manually run > > the doc-diff script. But anybody running "make clean" would > > probably want it to go away. > > > > Suggested-by: Eric Sunshine > > Signed-off-by: Jeff King > > --- > > diff --git a/Documentation/Makefile b/Documentation/Makefile > > @@ -332,6 +332,7 @@ clean: > > $(RM) manpage-base-url.xsl > > + $(RM) -r tmp-doc-diff > > Taking into consideration that people might be surprised and alarmed > to find "git worktree list" showing a worktree they didn't explicitly > create, would it make sense to do something like this? > > clean: > ... > -git worktree remove -f tmp-doc-diff 2>/dev/null > $(RM) -r tmp-doc-diff Seems reasonable. Again, I don't have a strong feeling. It's a little strange to me for the Makefile to be touching bits outside of the actual working tree. But then, creating a separate worktree in the first place is perhaps a little weird. I dunno. Maybe you are right that worktrees are a bad fit here. -Peff
Re: [PATCH v2 4/4] branch: make "-l" a synonym for "--list"
On Thu, Aug 30, 2018 at 01:29:53PM -0700, Junio C Hamano wrote:
> >> > I do not know if the documentation that is shipped in 2.20 should
> >> > talk about how the old world looked like, though. `-l` was a short
> >> > for `--create-reflog` is worth saying, but I do not see much value
> >> > in talking about the warning given in 2.19.
> >>
> >> I'm anticipating that there will be users in the wild with similar -l
> >> invocations, noting this helps them, because they'll be wondering what
> >> some script that does "git branch -l " is trying to do while
> >> reading our docs.
> >
> > I don't have a strong opinion either way. If we do mention it, it should
> > probably be short ("Until Git v2.20, the `-l` option was a synonym for
> > `--create-reflog").
>
> I agree that the short one would of course be good. I am on the
> fence about mentioning the warning only given in 2.19.
Yeah, I was confused about that part of the thread. Is there something
proposed to (additionally) go into v2.19? Ævar, can you elaborate?
-Peff
Re: [PATCH] doc/Makefile: remove tmp-doc-diff on "make clean"
On Thu, Aug 30, 2018 at 4:34 PM Eric Sunshine wrote: > Taking into consideration that people might be surprised and alarmed > to find "git worktree list" showing a worktree they didn't explicitly > create, would it make sense to do something like this? > > clean: > ... > -git worktree remove -f tmp-doc-diff 2>/dev/null > $(RM) -r tmp-doc-diff More accurately: -git worktree remove -f Documentation/tmp-doc-diff/worktree 2>/dev/null $(RM) -r tmp-doc-diff
Re: [PATCH] doc/Makefile: remove tmp-doc-diff on "make clean"
On Thu, Aug 30, 2018 at 3:55 PM Jeff King wrote: > The tmp-doc-diff directory isn't strictly a build product of > the Makefile, since it's only present if you manually run > the doc-diff script. But anybody running "make clean" would > probably want it to go away. > > Suggested-by: Eric Sunshine > Signed-off-by: Jeff King > --- > diff --git a/Documentation/Makefile b/Documentation/Makefile > @@ -332,6 +332,7 @@ clean: > $(RM) manpage-base-url.xsl > + $(RM) -r tmp-doc-diff Taking into consideration that people might be surprised and alarmed to find "git worktree list" showing a worktree they didn't explicitly create, would it make sense to do something like this? clean: ... -git worktree remove -f tmp-doc-diff 2>/dev/null $(RM) -r tmp-doc-diff
Re: [PATCH v2 4/4] branch: make "-l" a synonym for "--list"
Jeff King writes:
> On Thu, Aug 30, 2018 at 09:53:25PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> > In the SYNOPSIS section we still see "[-l]" listed; that also must
>> > be replaced with "--create-reflog", or just dropped, as that is the
>> > default.
>>
>> Oh yes, it seems all of the doc indeed wasn't updated!
>
> Sorry, this is my fault. Patch is below (which would go on top of
> jk/branch-l-1-repurpose).
Heh, reviewers who did not notice share the same blame. The patch
looks good. Thanks for a quick update.
>> > I do not know if the documentation that is shipped in 2.20 should
>> > talk about how the old world looked like, though. `-l` was a short
>> > for `--create-reflog` is worth saying, but I do not see much value
>> > in talking about the warning given in 2.19.
>>
>> I'm anticipating that there will be users in the wild with similar -l
>> invocations, noting this helps them, because they'll be wondering what
>> some script that does "git branch -l " is trying to do while
>> reading our docs.
>
> I don't have a strong opinion either way. If we do mention it, it should
> probably be short ("Until Git v2.20, the `-l` option was a synonym for
> `--create-reflog").
I agree that the short one would of course be good. I am on the
fence about mentioning the warning only given in 2.19.
> -- >8 --
> Subject: [PATCH] doc/git-branch: remove obsolete "-l" references
>
> The previous commit switched "-l" to meaning "--list", but a
> few vestiges of its prior meaning as "--create-reflog"
> remained:
>
> - the synopsis mentioned "-l" when creating a new branch;
> we can drop this entirely, as it has been the default
> for years
>
> - the --list command mentions the unfortunate "-l"
> confusion, but we've now fixed that
>
> Signed-off-by: Jeff King
> ---
> Documentation/git-branch.txt | 6 +-
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index 5552dfcec3..bf5316ffa9 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -14,7 +14,7 @@ SYNOPSIS
> [(--merged | --no-merged) []]
> [--contains []]
> [--points-at ] [--format=] [...]
> -'git branch' [--track | --no-track] [-l] [-f] []
> +'git branch' [--track | --no-track] [-f] []
> 'git branch' (--set-upstream-to= | -u ) []
> 'git branch' --unset-upstream []
> 'git branch' (-m | -M) []
> @@ -159,10 +159,6 @@ This option is only applicable in non-verbose mode.
> List branches. With optional `...`, e.g. `git
> branch --list 'maint-*'`, list only the branches that match
> the pattern(s).
> -+
> -This should not be confused with `git branch -l `,
> -which creates a branch named `` with a reflog.
> -See `--create-reflog` above for details.
>
> -v::
> -vv::
Re: [PATCH 0/9] worktree: fix bugs and broaden --force applicability
Jeff King writes:
> I suppose so. I don't think I've _ever_ used distclean, and I only
> rarely use "clean" (a testament to our Makefile's efforts to accurately
> track dependencies). I'd usually use "git clean" when I want something
> pristine (because I don't want to trust the Makefile at all).
I do not trust "git clean" all that much, and pre-cleaning with
"make distclean" and then running "git clean -x" has become my bad
habit. I jump around quite a bit during the day, which would end up
littering the working tree with *.o files that are only known to one
but not both of {maint,pu}/Makefile's distclean rules. I even do
"for i in pu maint master next; do git checkout $i; make distclean; done"
sometimes before running "git clean -x" ;-)
[PATCH v4 6/6] fetch: stop clobbering existing tags without --force
Change "fetch" to treat "+" in refspecs (aka --force) to mean we
should clobber a local tag of the same name.
This changes the long-standing behavior of "fetch" added in
853a3697dc ("[PATCH] Multi-head fetch.", 2005-08-20). Before this
change, all tag fetches effectively had --force enabled. See the
git-fetch-script code in fast_forward_local() with the comment:
> Tags need not be pointing at commits so there is no way to
> guarantee "fast-forward" anyway.
That commit and the rest of the history of "fetch" shows that the
"+" (--force) part of refpecs was only conceived for branch updates,
while tags have accepted any changes from upstream unconditionally and
clobbered the local tag object. Changing this behavior has been
discussed as early as 2011[1].
The current behavior doesn't make sense to me, it easily results in
local tags accidentally being clobbered. We could namespace our tags
per-remote and not locally populate refs/tags/*, but as with my
97716d217c ("fetch: add a --prune-tags option and fetch.pruneTags
config", 2018-02-09) it's easier to work around the current
implementation than to fix the root cause.
So this change implements suggestion #1 from Jeff's 2011 E-Mail[1],
"fetch" now only clobbers the tag if either "+" is provided as part of
the refspec, or if "--force" is provided on the command-line.
This also makes it nicely symmetrical with how "tag" itself works when
creating tags. I.e. we refuse to clobber any existing tags unless
"--force" is supplied. Now we can refuse all such clobbering, whether
it would happen by clobbering a local tag with "tag", or by fetching
it from the remote with "fetch".
Ref updates outside refs/{tags,heads/* are still still not symmetrical
with how "git push" works, as discussed in the recently changed
pull-fetch-param.txt documentation. This change brings the two
divergent behaviors more into line with one another. I don't think
there's any reason "fetch" couldn't fully converge with the behavior
used by "push", but that's a topic for another change.
One of the tests added in 31b808a032 ("clone --single: limit the fetch
refspec to fetched branch", 2012-09-20) is being changed to use
--force where a clone would clobber a tag. This changes nothing about
the existing behavior of the test.
1. https://public-inbox.org/git/2023221658.ga22...@sigill.intra.peff.net/
Signed-off-by: Ævar Arnfjörð Bjarmason
---
Documentation/pull-fetch-param.txt | 11 +++
builtin/fetch.c| 18 --
t/t5516-fetch-push.sh | 5 +++--
t/t5612-clone-refspec.sh | 4 ++--
4 files changed, 24 insertions(+), 14 deletions(-)
diff --git a/Documentation/pull-fetch-param.txt
b/Documentation/pull-fetch-param.txt
index ab9617ad01..47c832b17c 100644
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt
@@ -43,10 +43,13 @@ same rules apply for fetching as when pushing, see the
`...`
section of linkgit:git-push[1] for what those are. Exceptions to those
rules particular to 'git fetch' are noted below.
+
-Unlike when pushing with linkgit:git-push[1], any updates to
-`refs/tags/*` will be accepted without `+` in the refspec (or
-`--force`). The receiving promiscuously considers all tag updates from
-a remote to be forced fetches.
+Until Git version 2.20, and unlike when pushing with
+linkgit:git-push[1], any updates to `refs/tags/*` would be accepted
+without `+` in the refspec (or `--force`). The receiving promiscuously
+considered all tag updates from a remote to be forced fetches. Since
+Git version 2.20 updates to `refs/tags/*` work the same way as when
+pushing. I.e. any updates will be rejected without `+` in the refspec
+(or `--force`).
+
Unlike when pushing with linkgit:git-push[1], any updates outside of
`refs/{tags,heads}/*` will be accepted without `+` in the refspec (or
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b0706b3803..ed4ed9d8c4 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -667,12 +667,18 @@ static int update_local_ref(struct ref *ref,
if (!is_null_oid(>old_oid) &&
starts_with(ref->name, "refs/tags/")) {
- int r;
- r = s_update_ref("updating tag", ref, 0);
- format_display(display, r ? '!' : 't', _("[tag update]"),
- r ? _("unable to update local ref") : NULL,
- remote, pretty_ref, summary_width);
- return r;
+ if (force || ref->force) {
+ int r;
+ r = s_update_ref("updating tag", ref, 0);
+ format_display(display, r ? '!' : 't', _("[tag
update]"),
+ r ? _("unable to update local ref") :
NULL,
+ remote, pretty_ref, summary_width);
+ return r;
+ } else {
+ format_display(display, '!', _("[rejected]"),
[PATCH v4 4/6] push doc: correct lies about how push refspecs work
There's complex rules governing whether a push is allowed to take
place depending on whether we're pushing to refs/heads/*, refs/tags/*
or refs/not-that/*. See is_branch() in refs.c, and the various
assertions in refs/files-backend.c. (e.g. "trying to write non-commit
object %s to branch '%s'").
This documentation has never been quite correct, but went downhill
after dbfeddb12e ("push: require force for refs under refs/tags/",
2012-11-29) when we started claiming that couldn't be a tag
object, which is incorrect. After some of the logic in that patch was
changed in 256b9d70a4 ("push: fix "refs/tags/ hierarchy cannot be
updated without --force"", 2013-01-16) the docs weren't updated, and
we've had some version of documentation that confused whether
was a tag or not with whether would accept either an annotated
tag object or the commit it points to.
This makes the intro somewhat more verbose & complex, perhaps we
should have a shorter description here and split the full complexity
into a dedicated section. Very few users will find themselves needing
to e.g. push blobs or trees to refs/custom-namespace/* (or blobs or
trees at all), and that could be covered separately as an advanced
topic.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
Documentation/git-push.txt | 41 +-
Documentation/gitrevisions.txt | 7 +++---
2 files changed, 39 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 55277a9781..0f03d36f1e 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -74,12 +74,41 @@ without any `` on the command line. Otherwise,
missing
`:` means to update the same ref as the ``.
+
The object referenced by is used to update the reference
-on the remote side. By default this is only allowed if is not
-a tag (annotated or lightweight), and then only if it can fast-forward
-. By having the optional leading `+`, you can tell Git to update
-the ref even if it is not allowed by default (e.g., it is not a
-fast-forward.) This does *not* attempt to merge into . See
-EXAMPLES below for details.
+on the remote side. Whether this is allowed depends on where in
+`refs/*` the reference lives as described in detail below. Any
+such update does *not* attempt to merge into . See EXAMPLES
+below for details.
++
+The `refs/heads/*` namespace will only accept commit objects, and only
+if they can be fast-forwarded.
++
+The `refs/tags/*` namespace will accept any kind of object (as
+commits, trees and blobs can be tagged), and any changes to them will
+be rejected.
++
+It's possible to push any type of object to any namespace outside of
+`refs/{tags,heads}/*`. In the case of tags and commits, these will be
+treated as if they were the commits inside `refs/heads/*` for the
+purposes of whether the update is allowed.
++
+I.e. a fast-forward of commits and tags outside `refs/{tags,heads}/*`
+is allowed, even in cases where what's being fast-forwarded is not a
+commit, but a tag object which happens to point to a new commit which
+is a fast-forward of the commit the last tag (or commit) it's
+replacing. Replacing a tag with an entirely different tag is also
+allowed, if it points to the same commit, as well as pushing a peeled
+tag, i.e. pushing the commit that existing tag object points to, or a
+new tag object which an existing commit points to.
++
+Tree and blob objects outside of `refs/{tags,heads}/*` will be treated
+the same way as if they were inside `refs/tags/*`, any modification of
+them will be rejected.
++
+All of the rules described above about what's not allowed as an update
+can be overridden by adding an the optional leading `+` to a refspec
+(or using `--force` command line option). The only exception to this
+is that no amount of forcing will make the `refs/heads/*` namespace
+accept a non-commit object.
+
`tag ` means the same as `refs/tags/:refs/tags/`.
+
diff --git a/Documentation/gitrevisions.txt b/Documentation/gitrevisions.txt
index 1f6cceaefb..d407b7dee1 100644
--- a/Documentation/gitrevisions.txt
+++ b/Documentation/gitrevisions.txt
@@ -19,9 +19,10 @@ walk the revision graph (such as linkgit:git-log[1]), all
commits which are
reachable from that commit. For commands that walk the revision graph one can
also specify a range of revisions explicitly.
-In addition, some Git commands (such as linkgit:git-show[1]) also take
-revision parameters which denote other objects than commits, e.g. blobs
-("files") or trees ("directories of files").
+In addition, some Git commands (such as linkgit:git-show[1] and
+linkgit:git-push[1]) can also take revision parameters which denote
+other objects than commits, e.g. blobs ("files") or trees
+("directories of files").
include::revisions.txt[]
--
2.19.0.rc1.350.ge57e33dbd1
[PATCH v4 5/6] fetch: document local ref updates with/without --force
Refer to the new git-push(1) documentation about when ref updates are
and aren't allowed with and without --force, noting how "git-fetch"
differs from the behavior of "git-push".
Perhaps it would be better to split this all out into a new
gitrefspecs(7) man page, or present this information using tables.
In lieu of that, this is accurate, and fixes a big omission in the
existing refspec docs.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
Documentation/fetch-options.txt| 15 +-
Documentation/pull-fetch-param.txt | 32 +-
2 files changed, 37 insertions(+), 10 deletions(-)
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 8bc36af4b1..fa0a3151b3 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -68,11 +68,16 @@ endif::git-pull[]
-f::
--force::
- When 'git fetch' is used with `:`
- refspec, it refuses to update the local branch
- `` unless the remote branch `` it
- fetches is a descendant of ``. This option
- overrides that check.
+ When 'git fetch' is used with `:` refspec it may
+ refuse to update the local branch as discussed
+ifdef::git-pull[]
+ in the `` part of the linkgit:git-fetch[1]
+ documentation.
+endif::git-pull[]
+ifndef::git-pull[]
+ in the `` part below.
+endif::git-pull[]
+ This option overrides that check.
-k::
--keep::
diff --git a/Documentation/pull-fetch-param.txt
b/Documentation/pull-fetch-param.txt
index f1fb08dc68..ab9617ad01 100644
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt
@@ -33,11 +33,33 @@ name.
it requests fetching everything up to the given tag.
+
The remote ref that matches
-is fetched, and if is not an empty string, the local
-ref that matches it is fast-forwarded using .
-If the optional plus `+` is used, the local ref
-is updated even if it does not result in a fast-forward
-update.
+is fetched, and if is not an empty string, an attempt
+is made to update the local ref that matches it.
++
+Whether that update is allowed without `--force` depends on the ref
+namespace it's being fetched to, the type of object being fetched, and
+whether the update is considered to be a fast-forward. Generally, the
+same rules apply for fetching as when pushing, see the `...`
+section of linkgit:git-push[1] for what those are. Exceptions to those
+rules particular to 'git fetch' are noted below.
++
+Unlike when pushing with linkgit:git-push[1], any updates to
+`refs/tags/*` will be accepted without `+` in the refspec (or
+`--force`). The receiving promiscuously considers all tag updates from
+a remote to be forced fetches.
++
+Unlike when pushing with linkgit:git-push[1], any updates outside of
+`refs/{tags,heads}/*` will be accepted without `+` in the refspec (or
+`--force`), whether that's swapping e.g. a tree object for a blob, or
+a commit for another commit that's doesn't have the previous commit as
+an ancestor etc.
++
+As with pushing with linkgit:git-push[1], all of the rules described
+above about what's not allowed as an update can be overridden by
+adding an the optional leading `+` to a refspec (or using `--force`
+command line option). The only exception to this is that no amount of
+forcing will make the `refs/heads/*` namespace accept a non-commit
+object.
+
[NOTE]
When the remote branch you want to fetch is known to
--
2.19.0.rc1.350.ge57e33dbd1
[PATCH v4 3/6] fetch tests: add a test for clobbering tag behavior
The test suite only incidentally (and unintentionally) tested for the
current behavior of eager tag clobbering on "fetch". This is a
followup to 380efb65df ("push tests: assert re-pushing annotated
tags", 2018-07-31) which tests for it explicitly.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
t/t5516-fetch-push.sh | 24
1 file changed, 24 insertions(+)
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 69f7c9bfe6..3cde72ae47 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1011,6 +1011,30 @@ test_force_push_tag () {
test_force_push_tag "lightweight tag" "-f"
test_force_push_tag "annotated tag" "-f -a -mtag.message"
+test_force_fetch_tag () {
+ tag_type_description=$1
+ tag_args=$2
+
+ test_expect_success "fetch will clobber an existing
$tag_type_description" "
+ mk_test testrepo heads/master &&
+ mk_child testrepo child1 &&
+ mk_child testrepo child2 &&
+ (
+ cd testrepo &&
+ git tag testTag &&
+ git -C ../child1 fetch origin tag testTag &&
+ >file1 &&
+ git add file1 &&
+ git commit -m 'file1' &&
+ git tag $tag_args testTag &&
+ git -C ../child1 fetch origin tag testTag
+ )
+ "
+}
+
+test_force_fetch_tag "lightweight tag" "-f"
+test_force_fetch_tag "annotated tag" "-f -a -mtag.message"
+
test_expect_success 'push --porcelain' '
mk_empty testrepo &&
echo >.git/foo "To testrepo" &&
--
2.19.0.rc1.350.ge57e33dbd1
[PATCH v4 2/6] push tests: correct quoting in interpolated string
The quoted -m'msg' option is passed as a string to another function,
where due to interpolation it'll end up meaning the same as if we did
just did -m'msg' here.
In [1] this was pointed out to me, but in submitting [2] the patches I
missed this (since it was feedback on another patch I was holding
off), so this logic error landed in 380efb65df ("push tests: assert
re-pushing annotated tags", 2018-07-31).
Let's just remove the quotes, and use a string that doesn't need to be
quoted (-mtag.message is a bit less confusing than -mmsg). I could try
to chase after getting the quoting right here with multiple
backslashes, but I don't think it's worth it, and it makes things much
less readable.
1. https://public-inbox.org/git/xmqq4lgfcn5a@gitster-ct.c.googlers.com/
2. https://public-inbox.org/git/20180813192249.27585-1-ava...@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason
---
t/t5516-fetch-push.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 539c25aada..69f7c9bfe6 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1009,7 +1009,7 @@ test_force_push_tag () {
}
test_force_push_tag "lightweight tag" "-f"
-test_force_push_tag "annotated tag" "-f -a -m'msg'"
+test_force_push_tag "annotated tag" "-f -a -mtag.message"
test_expect_success 'push --porcelain' '
mk_empty testrepo &&
--
2.19.0.rc1.350.ge57e33dbd1
[PATCH v4 0/6] "git fetch" should not clobber existing tags without --force
Now that the tests for this have landed in master (in v3), and because I needed to rebase these for rolling out my own version based on v2.19.0-rc1, here's a re-roll which should address the (mostly doc) comments on the previous (v2) round. Ævar Arnfjörð Bjarmason (6): fetch: change "branch" to "reference" in --force -h output push tests: correct quoting in interpolated string fetch tests: add a test for clobbering tag behavior push doc: correct lies about how push refspecs work fetch: document local ref updates with/without --force fetch: stop clobbering existing tags without --force Documentation/fetch-options.txt| 15 +++ Documentation/git-push.txt | 41 +- Documentation/gitrevisions.txt | 7 ++--- Documentation/pull-fetch-param.txt | 35 + builtin/fetch.c| 20 ++- t/t5516-fetch-push.sh | 27 +++- t/t5612-clone-refspec.sh | 4 +-- 7 files changed, 120 insertions(+), 29 deletions(-) -- 2.19.0.rc1.350.ge57e33dbd1
[PATCH v4 1/6] fetch: change "branch" to "reference" in --force -h output
The -h output has been referring to the --force command as forcing the
overwriting of local branches, but since "fetch" more generally
fetches all sorts of references in all refs/ namespaces, let's talk
about forcing the update of a a "reference" instead.
This wording was initially introduced in 8320199873 ("Rewrite
builtin-fetch option parsing to use parse_options().", 2007-12-04).
Signed-off-by: Ævar Arnfjörð Bjarmason
---
builtin/fetch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 61bec5d213..b0706b3803 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -114,7 +114,7 @@ static struct option builtin_fetch_options[] = {
N_("append to .git/FETCH_HEAD instead of overwriting")),
OPT_STRING(0, "upload-pack", _pack, N_("path"),
N_("path to upload pack on remote end")),
- OPT__FORCE(, N_("force overwrite of local branch"), 0),
+ OPT__FORCE(, N_("force overwrite of local reference"), 0),
OPT_BOOL('m', "multiple", ,
N_("fetch from multiple remotes")),
OPT_SET_INT('t', "tags", ,
--
2.19.0.rc1.350.ge57e33dbd1
Re: [PATCH v2 1/1] builtin rebase: prepare for builtin rebase -i
On Thu, Aug 30, 2018 at 01:03:41PM +0200, Johannes Schindelin wrote:
> > Will replace by doing:
> >
> > $ git checkout js/rebase-in-c-5.5-work-with-rebase-i-in-c
> > $ git checkout HEAD^
> > $ git am -s mbox
> > $ git range-diff @{-1}...
> > $ git checkout -B @{-1}
> >
> > $ git checkout pk/rebase-i-in-c-6-final
> > $ git rebase --onto js/rebase-in-c-5.5-work-with-rebase-i-in-c \
> > js/rebase-in-c-5.5-work-with-rebase-i-in-c@{1} HEAD^0
> > $ git range-diff @{-1}...
> > $ git checkout -B @{-1}
> >
> > to update the two topics and then rebuilding the integration
> > branches the usual way. I also need to replace the "other" topic
> > used in this topic, so the actual procedure would be a bit more
> > involved than the above, though.
>
> Is there any reason why you avoid using `git rebase -ir` here? This should
> be so much easier via
>
> git checkout pk/rebase-i-in-c-6-final
> git rebase -ir js/rebase-in-c-5.5-work-with-rebase-i-in-c^
>
> and then inserting this at the appropriate position, followed by the `git
> range-diff @{-1}...`:
>
> git am -s mbox
> git update-ref js/rebase-in-c-5.5-work-with-rebase-i-in-c HEAD
Related discussion, including a fantasy tangent by me (downthread):
https://public-inbox.org/git/20180727080807.ga11...@sigill.intra.peff.net/#t
-Peff
Re: [PATCH v2 4/4] branch: make "-l" a synonym for "--list"
On Thu, Aug 30, 2018 at 09:53:25PM +0200, Ævar Arnfjörð Bjarmason wrote:
> > In the SYNOPSIS section we still see "[-l]" listed; that also must
> > be replaced with "--create-reflog", or just dropped, as that is the
> > default.
>
> Oh yes, it seems all of the doc indeed wasn't updated!
Sorry, this is my fault. Patch is below (which would go on top of
jk/branch-l-1-repurpose).
> > I do not know if the documentation that is shipped in 2.20 should
> > talk about how the old world looked like, though. `-l` was a short
> > for `--create-reflog` is worth saying, but I do not see much value
> > in talking about the warning given in 2.19.
>
> I'm anticipating that there will be users in the wild with similar -l
> invocations, noting this helps them, because they'll be wondering what
> some script that does "git branch -l " is trying to do while
> reading our docs.
I don't have a strong opinion either way. If we do mention it, it should
probably be short ("Until Git v2.20, the `-l` option was a synonym for
`--create-reflog").
-Peff
-- >8 --
Subject: [PATCH] doc/git-branch: remove obsolete "-l" references
The previous commit switched "-l" to meaning "--list", but a
few vestiges of its prior meaning as "--create-reflog"
remained:
- the synopsis mentioned "-l" when creating a new branch;
we can drop this entirely, as it has been the default
for years
- the --list command mentions the unfortunate "-l"
confusion, but we've now fixed that
Signed-off-by: Jeff King
---
Documentation/git-branch.txt | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 5552dfcec3..bf5316ffa9 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -14,7 +14,7 @@ SYNOPSIS
[(--merged | --no-merged) []]
[--contains []]
[--points-at ] [--format=] [...]
-'git branch' [--track | --no-track] [-l] [-f] []
+'git branch' [--track | --no-track] [-f] []
'git branch' (--set-upstream-to= | -u ) []
'git branch' --unset-upstream []
'git branch' (-m | -M) []
@@ -159,10 +159,6 @@ This option is only applicable in non-verbose mode.
List branches. With optional `...`, e.g. `git
branch --list 'maint-*'`, list only the branches that match
the pattern(s).
-+
-This should not be confused with `git branch -l `,
-which creates a branch named `` with a reflog.
-See `--create-reflog` above for details.
-v::
-vv::
--
2.19.0.rc1.546.g3fcb3c0d7c
[PATCH] doc/Makefile: remove tmp-doc-diff on "make clean"
The tmp-doc-diff directory isn't strictly a build product of the Makefile, since it's only present if you manually run the doc-diff script. But anybody running "make clean" would probably want it to go away. Suggested-by: Eric Sunshine Signed-off-by: Jeff King --- > Another fixup for jk/diff-rendered-docs,[...] And here's one more. I don't have a strong opinion on this myself, but it seems sensible. I doubt anybody cares overly much about the cost of an extra $(RM) during "make clean" (and if they do, we really ought to consider joining the existing ones into a single invocation). Documentation/Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/Makefile b/Documentation/Makefile index a42dcfc745..99a349ce9a 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -332,6 +332,7 @@ clean: $(RM) SubmittingPatches.txt $(RM) $(cmds_txt) $(mergetools_txt) *.made $(RM) manpage-base-url.xsl + $(RM) -r tmp-doc-diff $(MAN_HTML): %.html : %.txt asciidoc.conf $(QUIET_ASCIIDOC)$(RM) $@+ $@ && \ -- 2.19.0.rc1.546.g3fcb3c0d7c
Re: [PATCH v2 4/4] branch: make "-l" a synonym for "--list"
On Thu, Aug 30 2018, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > >>> +-l:: >>> --list:: >>> List branches. With optional `...`, e.g. `git >>> branch --list 'maint-*'`, list only the branches that match >> >> I think it's better to have something like this on top: >> >> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt >> index 5552dfcec3..a03cb1ebc9 100644 >> --- a/Documentation/git-branch.txt >> +++ b/Documentation/git-branch.txt >> @@ -163,6 +163,11 @@ This option is only applicable in non-verbose mode. >> This should not be confused with `git branch -l `, >> which creates a branch named `` with a reflog. >> See `--create-reflog` above for details. >> ++ >> + >> +Until Git version 2.20 `-l` was the short form of >> +`--create-reflog`. As of version 2.19 using it would warn about a >> +future deprecation. > > Doesn't your patch show a more grave issue with the current state of > 'next'? > > The sentence in the pre-context in your suggested patch says that > "--list" should not be confused with "git branch -l ", > but --list and -l are now synonyms in the new world order. > > Worse yet, '-l' is no longer a way to create the branch with a > reflog; in the new world, you would say "--create-reflog" if you > want to do so. "git branch -l foo" would list branches that match > that pattern 'foo'. > > In the SYNOPSIS section we still see "[-l]" listed; that also must > be replaced with "--create-reflog", or just dropped, as that is the > default. Oh yes, it seems all of the doc indeed wasn't updated! > I do not know if the documentation that is shipped in 2.20 should > talk about how the old world looked like, though. `-l` was a short > for `--create-reflog` is worth saying, but I do not see much value > in talking about the warning given in 2.19. I'm anticipating that there will be users in the wild with similar -l invocations, noting this helps them, because they'll be wondering what some script that does "git branch -l " is trying to do while reading our docs. Both our command-line options (plumbing or otherwise) and file various formats (e.g. I had a similar mention of version differences in my recent skipList patches) can be expected to be used across multiple git versions, by users who most likely are only browsing the latest version of the docs, not comparing how the manpage looked like in multiple git versions to see if an option meant something different, or if a format was documented as behaving differently.
Re: [PATCH 0/9] worktree: fix bugs and broaden --force applicability
On Thu, Aug 30, 2018 at 05:04:32AM -0400, Eric Sunshine wrote: > On Thu, Aug 30, 2018 at 3:54 AM Jeff King wrote: > > Subject: [PATCH] doc-diff: force worktree add > > > > We avoid re-creating our temporary worktree if it's already > > there. But we may run into a situation where the worktree > > has been deleted, but an entry still exists in > > $GIT_DIR/worktrees. > > Can "clean" or "distclean" also be augmented to remove this worktree > (and directory)? I guess that "distclean" would make more sense than > "clean"(?). I suppose so. I don't think I've _ever_ used distclean, and I only rarely use "clean" (a testament to our Makefile's efforts to accurately track dependencies). I'd usually use "git clean" when I want something pristine (because I don't want to trust the Makefile at all). -Peff
Re: Git in Outreachy Dec-Mar?
On Thu, Aug 30, 2018 at 02:18:19PM +0200, Ævar Arnfjörð Bjarmason wrote: > > It doesn't need to be. As far as I know, the main reasons (from the > > perspective of a project) to do it through Outreachy are: > > > > - being part of a larger program generates attention and gets the > >interest of intern candidates (free advertising, if you will) > > I was wondering if we couldn't do it through Outreachy *and* also do our > own advertisements / possibly recruit candidates outside of the > Outreachy pool. In that case we'd still get the attention/outreach > benefits, in addition to our own... True. I'd worry about spreading our mentor resources too thinly (which I think are probably a bigger bottleneck than actual money). But I guess you're proposing to issue a larger call for candidates, and then we pick from the result (so in the end we'd end up with the same number of actual interns, just from a bigger pool). > Yup, but just as a clarifying point here wouldn't the participants also > get all the same benefits of this in the case of Outreachy+OurOwnProgram > if we ran OurOwnProgram concurrently to Outreachy? > > I.e. I was assuming that once candidates are "handed off" to a project > they're communicating within that project (possibly with other > candidates), and Outreachy is no longer very involved (except maybe for > progress reports / final report, but wouldn't we also do that for a > OurOwnProgram?). > > I may have that completely wrong though, which is why I'm asking, which > b.t.w. I'm doing mostly just to get an idea of how what Outreachy's role > is in this exactly, not to strongly advocate for a OurOwnProgram. I think there _is_ some contact and group resources between Outreachy and the interns. But I'm actually not sure of the extent. I know they encouraged interns to blog (and read each other's blogs). I don't know if there's an intern mailing list, irc, etc. I had the impression that there is, but I don't actually know the details. > > - it naturally limits the candidate pool to under-represented groups > > (which is the whole point of the program, but if you don't > > actually care about that, then it's just a complication) > > I'm fine with doing selection discrimination of under-represented groups > through such a program. Particularly if, as you mention, there's > earmarked funding for it which otherwise might not be available, so it's > not zero-sum when it comes to a hypothetical alternative of casting a > wider net of our own (and as you mention, that would be more work). Yeah, just for reference, my "you" there was a hypothetical "one might or might not care about...", not responding to your particular email. > I do think it's unfortunate that the selection criteria for the program > privileges U.S. citizens and U.S. residents above other people, > particularly since they're also accepting worldwide candidates (and > we've had at least one non-American participant that I know about), so > it's not e.g. for U.S. administrative or tax reasons as one might expect > if they only accepted Americans. I assume you mean this bit from the eligibility rules: You must meet one of the following criteria: - You live any where in the world and you identify as a woman (cis or trans), trans man, or genderqueer person (including genderfluid or genderfree). - You live in the United States or you are a U.S. national or permanent resident living abroad, AND you are a person of any gender who is Black/African American, Hispanic/Latin@, Native American/American Indian, Alaska Native, Native Hawaiian, or Pacific Islander So there are more categories for the US, but I think that is largely because under-representation is somewhat regional. Being black in the US is different than being black in Africa. Certainly one could argue that Africa as a whole is under-represented in the tech world, but I think you'd probably need to draw different boundaries in different places if you want to extend opportunities to those who are least likely to already have them. I don't know what those groupings would look like in, say, Europe. If you're suggesting that the program would be better off having region-specific rules for more regions, I'd certainly agree with that. I don't know if it's something the Outreachy folks have considered or discussed; it might be worth bringing it up. -Peff
Re: feature request: allow commit.email config setting
On 2018-08-30 20:13, Eric Sunshine wrote: > On Thu, Aug 30, 2018 at 7:26 AM Rasmus Villemoes > wrote: >> I can set GIT_COMMITTER_EMAIL in the environment, but that is >> rather inconvenient, since that means I have to remember to do that in >> the shell I'm using for that particular project, and I can't use that >> shell for other projects. So it would be really nice if I could set >> commit.email = $private-email in the local .git/config for that >> particular project. > > Aside from modifying Git itself to support such a use-case, another > (perhaps more pragmatic) approach would be to use a tool, such as > direnv[1], which automatically sets environment variables for you > depending upon your current working directory, or just use some ad-hoc > shell programming to achieve the same (for instance, [2]). Thanks for the hint! I've actually had "git" as a function in my .bashrc for a long time, for implementing a ~/.githistory to help remember the sometimes rather complex git invocations, and keeping track of the context ($cwd, current branch, etc.) they were used in. It should be trivial to hook the environment settings based on $cwd into that. The only problem is that that gives me much less incentive to work on implementing the config support in git, but if I'm the only one with a use case, that's probably just as well. Rasmus
Re: Possible bug: identical lines added/removed in git diff
On Thu, Aug 30, 2018 at 12:20 PM Jeff King wrote: > > On Thu, Aug 30, 2018 at 12:16:22PM -0700, Stefan Beller wrote: > > > On Wed, Aug 29, 2018 at 7:54 PM Jeff King wrote: > > > > > > On Wed, Aug 29, 2018 at 10:10:25PM -0400, Gabriel Holodak wrote: > > > > > > > > Could you cut down to a real minimal reproduction, i.e. just these 20 > > > > > lines or so? > > > > > > > > I'm working on getting down to a minimal reproduction, a few lines at > > > > a time. One thing that seems strange: as I've removed lines, there are > > > > a bunch of lines that don't matter. Then I'll find some lines that, if > > > > removed, completely fix the issue. But the ordering for these > > > > apparently important lines doesn't matter. They just have to be > > > > somewhere in the file to cause the duplicated diffs. > > > > > > > > I'll upload again when I've figured out all the unimportant lines to > > > > remove. > > > > > > Yeah, I reproduced based on your initial post, but noticed that when I > > > cut it down the problem went away. > > > > Oh, I had to look further down than I did initially. Now I can reproduce it > > from the initial data as well. > > > > Note that it goes away with --minimal. > > That's interesting. I did wonder if this was in fact a bug, or simply > that Myers does not promise to find the absolute minimal diff. I'm > _still_ not sure, especially because the minimization is so obvious in > this case (literally the first "-" and the first "+" line of a > contiguous hunk are identical). The `Myers` (our default) diff algorithm is really the Myers algorithm + a heuristic that cuts off the long tail when it is very costly to compute the minimal diff. The `minimal` diff is the true Myers algorithm and I'd vouch for its correctness and being the minimal number of lines in the diff output. The Myers is implemented before https://github.com/git/git/blob/master/xdiff/xdiffi.c#L135 and the heuristics is after that line. > > I have a patch cooking (which was sent out as > > https://public-inbox.org/git/20180810221857.87399-1-sbel...@google.com/) > > > > and one of the weaknesses in that patch is the lack of explanation on > > when the heuristic is applied as I have not fully understood it yet. > > I'm not sure I understand it either. But at least knowing that --minimal > changes the output gives a lead for investigation (I don't really have > time to dig into it in the next few days, though). An interesting (to me) approach for digging into that would include finding these examples at scale, which I presented in https://public-inbox.org/git/20180810001010.58870-1-sbel...@google.com/ but I guess reading the code would work just as fine. Thanks, Stefan
Re: Git in Outreachy Dec-Mar?
On Thu, Aug 30, 2018 at 01:46:00PM +0200, Johannes Schindelin wrote: > On Wed, 29 Aug 2018, Jeff King wrote: > > > - it naturally limits the candidate pool to under-represented groups > > (which is the whole point of the program, but if you don't > > actually care about that, then it's just a complication) > > > > So IMHO it's easily worth the trouble. > > I am willing to mentor, and the only reason that kept me from already > stepping forward and trying to brush up the landing page is this concern: > traditionally, we (as in: the core Git contributors) have been less than > successful in attracting and retaining contributors from under-represented > groups. I don't think any regular reader of this mailing list can deny > that. > > And while I find it very important to reach out (there are just *so* many > benefits to having a more diverse team), I have to ask *why* we are so > unsuccessful. As long as we do not even know the answer to that, is it > even worth pursuing Outreachy? > > I mean, if we make serious mistakes here, without even realizing, that > directly lead to being stuck in our old bubble, then we are prone to > simply repeat those mistakes over and over and over again. And that would > just be a waste of our time, *and* a big de-motivator for the Outreachy > students. > > What's your take on this? My feeling is that our lack of diversity has less to do with driving out diverse candidates, and more that they do not join in the first place. Which isn't to say we _wouldn't_ drive out diversity, but that I'm not sure we have very good data on what happens in that second stage. If we can use the program to overcome "step 1", that helps us get that data (and hopefully react to it in time to be useful, and not just use the candidate as a guinea pig; I agree there is the possibility of doing more harm than good to a student who becomes de-motivated). That leaves aside the question of whether things we are doing prevent people from participating in the first place. I'm certainly open to that idea, but I think it's a separate discussion. -Peff
Re: Possible bug: identical lines added/removed in git diff
On Thu, Aug 30, 2018 at 12:16:22PM -0700, Stefan Beller wrote: > On Wed, Aug 29, 2018 at 7:54 PM Jeff King wrote: > > > > On Wed, Aug 29, 2018 at 10:10:25PM -0400, Gabriel Holodak wrote: > > > > > > Could you cut down to a real minimal reproduction, i.e. just these 20 > > > > lines or so? > > > > > > I'm working on getting down to a minimal reproduction, a few lines at > > > a time. One thing that seems strange: as I've removed lines, there are > > > a bunch of lines that don't matter. Then I'll find some lines that, if > > > removed, completely fix the issue. But the ordering for these > > > apparently important lines doesn't matter. They just have to be > > > somewhere in the file to cause the duplicated diffs. > > > > > > I'll upload again when I've figured out all the unimportant lines to > > > remove. > > > > Yeah, I reproduced based on your initial post, but noticed that when I > > cut it down the problem went away. > > Oh, I had to look further down than I did initially. Now I can reproduce it > from the initial data as well. > > Note that it goes away with --minimal. That's interesting. I did wonder if this was in fact a bug, or simply that Myers does not promise to find the absolute minimal diff. I'm _still_ not sure, especially because the minimization is so obvious in this case (literally the first "-" and the first "+" line of a contiguous hunk are identical). > I have a patch cooking (which was sent out as > https://public-inbox.org/git/20180810221857.87399-1-sbel...@google.com/) > > and one of the weaknesses in that patch is the lack of explanation on > when the heuristic is applied as I have not fully understood it yet. I'm not sure I understand it either. But at least knowing that --minimal changes the output gives a lead for investigation (I don't really have time to dig into it in the next few days, though). -Peff
Re: Possible bug: identical lines added/removed in git diff
On Wed, Aug 29, 2018 at 7:54 PM Jeff King wrote: > > On Wed, Aug 29, 2018 at 10:10:25PM -0400, Gabriel Holodak wrote: > > > > Could you cut down to a real minimal reproduction, i.e. just these 20 > > > lines or so? > > > > I'm working on getting down to a minimal reproduction, a few lines at > > a time. One thing that seems strange: as I've removed lines, there are > > a bunch of lines that don't matter. Then I'll find some lines that, if > > removed, completely fix the issue. But the ordering for these > > apparently important lines doesn't matter. They just have to be > > somewhere in the file to cause the duplicated diffs. > > > > I'll upload again when I've figured out all the unimportant lines to remove. > > Yeah, I reproduced based on your initial post, but noticed that when I > cut it down the problem went away. Oh, I had to look further down than I did initially. Now I can reproduce it from the initial data as well. Note that it goes away with --minimal. I have a patch cooking (which was sent out as https://public-inbox.org/git/20180810221857.87399-1-sbel...@google.com/) and one of the weaknesses in that patch is the lack of explanation on when the heuristic is applied as I have not fully understood it yet.
Re: [PATCH 2/5] t5303: test some corrupt deltas
On Thu, Aug 30, 2018 at 11:50:56AM -0700, Junio C Hamano wrote: > Jeff King writes: > > > I can re-roll, or even prepare a patch on top (it's sufficiently subtle > > that it may merit calling out explicitly in a commit). > > Yeah, I tend to agree with your reasoning to do it on top as a > separate patch. Here it is, then. -- >8 -- Subject: [PATCH 6/5] t5303: use printf to generate delta bases The exact byte count of the delta base file is important. The test-delta helper will feed it to patch_delta(), which will barf if it doesn't match the size byte given in the delta. Using "echo" may end up with unexpected line endings on some platforms (e.g,. "\r\n" instead of just "\n"). This actually wouldn't cause the test to fail (since we already expect test-delta to complain about these bogus deltas), but would mean that we're not exercising the code we think we are. Let's use printf instead (which we already trust to give us byte-perfect output when we generate the deltas). While we're here, let's tighten the 5-byte result size used in the "truncated copy parameters" test. This just needs to have enough room to attempt to parse the bogus copy command, meaning 2 is sufficient. Using 5 was arbitrary and just copied from the base size; since those no longer match, it's simply confusing. Let's use a more meaningful number. Signed-off-by: Jeff King --- t/t5303-pack-corruption-resilience.sh | 20 ++-- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh index b68bbeedcc..41e6dc4dcf 100755 --- a/t/t5303-pack-corruption-resilience.sh +++ b/t/t5303-pack-corruption-resilience.sh @@ -327,15 +327,15 @@ test_expect_success \ 'printf "\0\1\2XX" > too_big_literal && test_must_fail test-tool delta -p /dev/null too_big_literal /dev/null' -# \5 - five bytes in base +# \4 - four bytes in base # \1 - one byte in result # \221 - copy, one byte offset, one byte size # \0 - copy from offset 0 # \2 - copy two bytes (one too many) test_expect_success \ 'apply delta with too many copied bytes' \ -'printf "\5\1\221\0\2" > too_big_copy && - echo base >base && +'printf "\4\1\221\0\2" > too_big_copy && + printf base >base && test_must_fail test-tool delta -p base too_big_copy /dev/null' # \0 - empty base @@ -356,8 +356,8 @@ test_expect_success \ 'printf "\0\1\221\0\1" > truncated_base && test_must_fail test-tool delta -p /dev/null truncated_base /dev/null' -# \5 - five bytes in base -# \5 - five bytes in result +# \4 - four bytes in base +# \2 - two bytes in result # \1 - one literal byte (X) # \221 - copy, one byte offset, one byte size #(offset/size missing) @@ -366,8 +366,8 @@ test_expect_success \ # delta size check. test_expect_success \ 'apply delta with truncated copy parameters' \ -'printf "\5\5\1X\221" > truncated_copy_delta && - echo base >base && +'printf "\4\2\1X\221" > truncated_copy_delta && + printf base >base && test_must_fail test-tool delta -p base truncated_copy_delta /dev/null' # \0 - empty base @@ -379,7 +379,7 @@ test_expect_success \ 'printf "\0\1\1X\1" > tail_garbage_literal && test_must_fail test-tool delta -p /dev/null tail_garbage_literal /dev/null' -# \5 - five bytes in base +# \4 - four bytes in base # \1 - one byte in result # \1 - one literal byte (X) # \221 - copy, one byte offset, one byte size @@ -387,8 +387,8 @@ test_expect_success \ # \1 - copy 1 byte test_expect_success \ 'apply delta with trailing garbage copy' \ -'printf "\5\1\1X\221\0\1" > tail_garbage_copy && - echo base >base && +'printf "\4\1\1X\221\0\1" > tail_garbage_copy && + printf base >base && test_must_fail test-tool delta -p /dev/null tail_garbage_copy /dev/null' # \0 - empty base -- 2.19.0.rc1.546.g3fcb3c0d7c
RE: Automatic core.autocrlf?
On August 30, 2018 2:57 PM, Torsten Bögershausen wrote: > On Thu, Aug 30, 2018 at 09:57:52AM -0500, Robert Dailey wrote: > > On Wed, Aug 29, 2018 at 11:54 PM Jonathan Nieder > wrote: > > > > > > Hi, > > > > > > Robert Dailey wrote: > > > > > > > Is there an 'auto' setting for the 'core.autocrlf' config? Reason > > > > I ask is, I want that setting to be 'input' on linux but 'true' on > > > > Windows. > > > > > > Others are exploring your question about the configuration language, > > > but I want to emphasize some other ramifications. > > > > > > Why do we still have 'core.autocrlf'? Do 'core.eol' and related > > > settings take care of that need, or is autocrlf still needed? If > > > core.eol etc do not take care of this need, what should we do to get > > > them to? > > > > > > Thanks, after having run into a few too many autocrlf-related > > > messes, Jonathan > > > > From my perspective, the confusion is due to the evolution of the > > feature. There's multiple ways to control EOL handling but most of it > > is legacy/backward compatibility, I think. core.autocrlf is a > > fall-back for repos that do not have a .gitattributes. Because > > .gitattributes is optional by design, I'm not sure if getting rid of > > the config options is a good idea. > > Good summary. My original plan was to try to "make obsolete"/retire and > phase out core.autocrlf completely. > However, since e.g. egit/jgit uses it > (they don't have support for .gitattributes at all) I am not sure if this is a good > idea either. Opinions are welcome. > > > > But your point did make me think > > about how `core.autocrlf = true` should probably be a system config > > default for the Git for Windows project. The default for that value > > should be platform-defined. That would make it automatically work the > > way I want, and might solve a lot of the issues where people are > > committing CRLF into repositories on Windows. > > Unless I am wrong, that had been the default a long time ago: > Git for Windows (at that time msysgit) had core.autocrlf=true by default. > While this is a good choice for many repos, some people prefer > core.autocrlf=input. > Others just commit files for Windows-based repos with CRLF, and the > advantage is, that "git diff" doesn't show "^M" somewhere. > > I allways encourage people to set up a .gitattributes file. > Does anybody thinks that we can make core.autocrlf obsolete ? The last time I checked, EGit does not set this by default. ECLIPSE Oxygen 3A/EGit-JGit 5.0.1, when running on Windows, creates core.filemode=false, core.logallrefupdates=true, repositoryformatversion=0, symlinks=false. Some SourceTree versions that predate the newer SourceTreeApp are somewhat stuck on older embedded versions of git, but that may not be relevant. Personally, I would seriously like to drop core.autocrlf and just have everyone on LF EOL characters. I get frequently burnt by this despite knowing better. Cheers, Randall -- Brief whoami: NonStop developer since approximately 2112884442 UNIX developer since approximately 421664400 -- In my real life, I talk too much.
Re: Automatic core.autocrlf?
On Thu, Aug 30, 2018 at 09:57:52AM -0500, Robert Dailey wrote: > On Wed, Aug 29, 2018 at 11:54 PM Jonathan Nieder wrote: > > > > Hi, > > > > Robert Dailey wrote: > > > > > Is there an 'auto' setting for the 'core.autocrlf' config? Reason I > > > ask is, I want that setting to be 'input' on linux but 'true' on > > > Windows. > > > > Others are exploring your question about the configuration language, > > but I want to emphasize some other ramifications. > > > > Why do we still have 'core.autocrlf'? Do 'core.eol' and related > > settings take care of that need, or is autocrlf still needed? If > > core.eol etc do not take care of this need, what should we do to get > > them to? > > > > Thanks, after having run into a few too many autocrlf-related messes, > > Jonathan > > From my perspective, the confusion is due to the evolution of the > feature. There's multiple ways to control EOL handling but most of it > is legacy/backward compatibility, I think. core.autocrlf is a > fall-back for repos that do not have a .gitattributes. Because > .gitattributes is optional by design, I'm not sure if getting rid of > the config options is a good idea. Good summary. My original plan was to try to "make obsolete"/retire and phase out core.autocrlf completely. However, since e.g. egit/jgit uses it (they don't have support for .gitattributes at all) I am not sure if this is a good idea either. Opinions are welcome. > But your point did make me think > about how `core.autocrlf = true` should probably be a system config > default for the Git for Windows project. The default for that value > should be platform-defined. That would make it automatically work the > way I want, and might solve a lot of the issues where people are > committing CRLF into repositories on Windows. Unless I am wrong, that had been the default a long time ago: Git for Windows (at that time msysgit) had core.autocrlf=true by default. While this is a good choice for many repos, some people prefer core.autocrlf=input. Others just commit files for Windows-based repos with CRLF, and the advantage is, that "git diff" doesn't show "^M" somewhere. I allways encourage people to set up a .gitattributes file. Does anybody thinks that we can make core.autocrlf obsolete ?
Re: [PATCH 2/5] t5303: test some corrupt deltas
Jeff King writes: > I can re-roll, or even prepare a patch on top (it's sufficiently subtle > that it may merit calling out explicitly in a commit). Yeah, I tend to agree with your reasoning to do it on top as a separate patch.
Re: [PATCH v2 4/4] branch: make "-l" a synonym for "--list"
Ævar Arnfjörð Bjarmason writes: >> +-l:: >> --list:: >> List branches. With optional `...`, e.g. `git >> branch --list 'maint-*'`, list only the branches that match > > I think it's better to have something like this on top: > > diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt > index 5552dfcec3..a03cb1ebc9 100644 > --- a/Documentation/git-branch.txt > +++ b/Documentation/git-branch.txt > @@ -163,6 +163,11 @@ This option is only applicable in non-verbose mode. > This should not be confused with `git branch -l `, > which creates a branch named `` with a reflog. > See `--create-reflog` above for details. > ++ > + > +Until Git version 2.20 `-l` was the short form of > +`--create-reflog`. As of version 2.19 using it would warn about a > +future deprecation. Doesn't your patch show a more grave issue with the current state of 'next'? The sentence in the pre-context in your suggested patch says that "--list" should not be confused with "git branch -l ", but --list and -l are now synonyms in the new world order. Worse yet, '-l' is no longer a way to create the branch with a reflog; in the new world, you would say "--create-reflog" if you want to do so. "git branch -l foo" would list branches that match that pattern 'foo'. In the SYNOPSIS section we still see "[-l]" listed; that also must be replaced with "--create-reflog", or just dropped, as that is the default. I do not know if the documentation that is shipped in 2.20 should talk about how the old world looked like, though. `-l` was a short for `--create-reflog` is worth saying, but I do not see much value in talking about the warning given in 2.19. Thanks.
Re: [PATCH 2/5] t5303: test some corrupt deltas
On Thu, Aug 30, 2018 at 02:42:01PM -0400, Jeff King wrote: > > Would "echo base >base" give us 5-byte long base even on Windows? > > Or the test does not care if it is either "base\n" or "base\r\n"? > > > > Just double-checking. > > Good question. On the first one, I don't know. On the second one, yes, > it does matter. We'd feed "6" to patch_delta(), and it would complain > about the mismatch before actually hitting the code we're trying to > exercise. The test would still pass (the error result is the same either > way), but would quietly not test what we wanted. > > Maybe something like this to be on the safe side? That could be squashed into patch 2. Patch 4 would need this one additional case: diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh index 0c537958e7..e91d6f5770 100755 --- a/t/t5303-pack-corruption-resilience.sh +++ b/t/t5303-pack-corruption-resilience.sh @@ -379,7 +379,7 @@ test_expect_success \ 'printf "\0\1\1X\1" > tail_garbage_literal && test_must_fail test-tool delta -p /dev/null tail_garbage_literal /dev/null' -# \5 - five bytes in base +# \4 - four bytes in base # \1 - one byte in result # \1 - one literal byte (X) # \221 - copy, one byte offset, one byte size @@ -387,8 +387,8 @@ test_expect_success \ # \1 - copy 1 byte test_expect_success \ 'apply delta with trailing garbage copy' \ -'printf "\5\1\1X\221\0\1" > tail_garbage_copy && - echo base >base && +'printf "\4\1\1X\221\0\1" > tail_garbage_copy && + printf base >base && test_must_fail test-tool delta -p /dev/null tail_garbage_copy /dev/null' # \0 - empty base I can re-roll, or even prepare a patch on top (it's sufficiently subtle that it may merit calling out explicitly in a commit). -Peff
Re: [PATCH 2/5] t5303: test some corrupt deltas
On Thu, Aug 30, 2018 at 10:38:21AM -0700, Junio C Hamano wrote: > Jeff King writes: > > > +test_expect_success \ > > +'apply delta with too many copied bytes' \ > > +'printf "\5\1\221\0\2" > too_big_copy && > > + echo base >base && > > + test_must_fail test-tool delta -p base too_big_copy /dev/null' > > Would "echo base >base" give us 5-byte long base even on Windows? > Or the test does not care if it is either "base\n" or "base\r\n"? > > Just double-checking. Good question. On the first one, I don't know. On the second one, yes, it does matter. We'd feed "6" to patch_delta(), and it would complain about the mismatch before actually hitting the code we're trying to exercise. The test would still pass (the error result is the same either way), but would quietly not test what we wanted. Maybe something like this to be on the safe side? (note that we can leave the \5 in the result size of the "truncated copy parameters" test; it really just needs to be larger than 1). --- diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh index 912e659acf..e80934a18e 100755 --- a/t/t5303-pack-corruption-resilience.sh +++ b/t/t5303-pack-corruption-resilience.sh @@ -327,15 +327,15 @@ test_expect_success \ 'printf "\0\1\2XX" > too_big_literal && test_must_fail test-tool delta -p /dev/null too_big_literal /dev/null' -# \5 - five bytes in base +# \4 - four bytes in base # \1 - one byte in result # \221 - copy, one byte offset, one byte size # \0 - copy from offset 0 # \2 - copy two bytes (one too many) test_expect_success \ 'apply delta with too many copied bytes' \ -'printf "\5\1\221\0\2" > too_big_copy && - echo base >base && +'printf "\4\1\221\0\2" > too_big_copy && + printf base >base && test_must_fail test-tool delta -p base too_big_copy /dev/null' # \0 - empty base @@ -356,7 +356,7 @@ test_expect_success \ 'printf "\0\1\221\0\1" > truncated_base && test_must_fail test-tool delta -p /dev/null truncated_base /dev/null' -# \5 - five bytes in base +# \4 - four bytes in base # \5 - five bytes in result # \1 - one literal byte (X) # \221 - copy, one byte offset, one byte size @@ -366,8 +366,8 @@ test_expect_success \ # delta size check. test_expect_failure \ 'apply delta with truncated copy parameters' \ -'printf "\5\5\1X\221" > truncated_copy_delta && - echo base >base && +'printf "\4\5\1X\221" > truncated_copy_delta && + printf base >base && test_must_fail test-tool delta -p base truncated_copy_delta /dev/null' test_done
Re: feature request: allow commit.email config setting
On Thu, Aug 30, 2018 at 7:26 AM Rasmus Villemoes wrote: > I can set GIT_COMMITTER_EMAIL in the environment, but that is > rather inconvenient, since that means I have to remember to do that in > the shell I'm using for that particular project, and I can't use that > shell for other projects. So it would be really nice if I could set > commit.email = $private-email in the local .git/config for that > particular project. Aside from modifying Git itself to support such a use-case, another (perhaps more pragmatic) approach would be to use a tool, such as direnv[1], which automatically sets environment variables for you depending upon your current working directory, or just use some ad-hoc shell programming to achieve the same (for instance, [2]). [1]: https://direnv.net [2]: https://stackoverflow.com/questions/14462591/set-environmental-variables-in-a-particular-directory
Re: [PATCH 2/5] t5303: test some corrupt deltas
Jeff King writes: > +test_expect_success \ > +'apply delta with too many copied bytes' \ > +'printf "\5\1\221\0\2" > too_big_copy && > + echo base >base && > + test_must_fail test-tool delta -p base too_big_copy /dev/null' Would "echo base >base" give us 5-byte long base even on Windows? Or the test does not care if it is either "base\n" or "base\r\n"? Just double-checking.
Re: [PATCH v3] checkout: optimize "git checkout -b "
Hi Duy, On Tue, Aug 21, 2018 at 7:52 AM Duy Nguyen wrote: > > On Mon, Aug 20, 2018 at 8:16 PM Elijah Newren wrote: > > Playing with sparse-checkout, it feels to me like a half-baked > > feature. It seems like it required too much manual work, and it was > > sometimes hard to tell if I was misunderstanding configuration rules > > or I was just running into bugs in the code. I think I hit both but I > > didn't really want to get side-tracked further, yet. (I do want to > > eventually come back to it.) The only reason someone would go through > > that pain is if it provided massive performance benefits. > > In my defense it was one of my first contribution when I was naiver > and basically an evolution of "git update-index --assume-unchanged". I > have something in the queue to improve/complement sparse-checkout but > my last update on that branch was 2.5 years ago, so it's not coming > soon. > > I'd love to year how sparse checkout could be improved, or even > replaced. I think we still have to have some configuration rules, and > yes the flexibility of sparse checkout (or gitignore to be precise) > rules is a double-edged sword. Sorry for taking a while to respond, and if what I said came across harshly. I agree that the flexibility of the rules makes it more complicated, though I think a bigger issue may be that the feature is hard to make smooth unless coupled to something like partial clones. Work on that is ongoing. Anyway, in an attempt to be helpful, here were some of the pain points I ran across: - The fact that documentation could only be found in a low-level plumbing command like read-tree made discoverability hard. Why would folks think to look there? (I can't remember if I had to google it or just grepped around the git source code to find it.) - Needing to use read-tree, which isn't something most users are familiar with, makes for a learning curve. I may know what some of the flags in read-tree do, but users will puzzle over which things on the page happen to be relevant to them -- especially since the section on sparse checkouts don't specify how read-tree should be invoked after .git/info/sparse-checkout is populated. Even I couldn't guess what I was supposed to ran and just googled for hints. Here's some possible failures, as users guess which flags to pass: $ git read-tree warning: read-tree: emptying the index with no arguments is deprecated; use --empty $ git read-tree HEAD # Oops, doesn't update the working directory $ git read-tree -u HEAD # Doesn't do any updates either $ git read-tree -mu HEAD # Works...but make user think "Why/what am I merging?!?" - I actually misunderstood or misread the documentation about undoing sparse checkouts and failed multiple times. I think I nuked the index then did a 'git reset --hard HEAD'. Re-reading, it looks like you did explain it, and I don't remember why/how I missed it the first time around, but I did. - I either failed to grasp how to specify negative refs or botched something else up. I tried digging for a while to figure out how to exclude my massive directory, but was always met with: error: Sparse checkout leaves no entry on working directory I spent a while trying to figure out what I did wrong, but gave up after half an hour or so since I wasn't trying to use the feature for real and just specified the files I wanted to keep instead. Re-reading the docs, it looks like you specified how to do this, and re-trying now it works, but I repeatedly passed over the '/*' in the docs and read it as either bad formatting or a highlight of the next line rather than as important literal text. So, maybe part of my problem is that I just can't read. :-)
Re: improved diff tool
On Thu, Aug 30, 2018 at 4:33 AM Johannes Schindelin wrote: > > Hi Piers, > > On Thu, 30 Aug 2018, Piers Titus van der Torren wrote: > > > I've created a diff algorithm that focuses on creating readable diffs, > > see https://github.com/pierstitus/klondiff > > Looks intriguing. Yes it does. The diff of c07c0923 looks quite interesting as it shows what was intended to happen (indented a lot of code as it was wrapped). Playing around with that commit and some recently added switches --color-moved --color-moved-ws=allow-indentation-change which is supposed to solve a similar problem did not yield as good results. > > The git integration as an external diff command works quite well, > > though it would be nice to integrate it deeper in git, and also in > > git-gui and gitk. Any way to use ext-diff from the gui tools? > > Git GUI and gitk are both Tcl/Tk programs, and will need quite a bit of > work to accommodate for your diff mode. It would be awesome to have gitk/git gui working with more diff options. > > To put things into perspective: the `--color-words` mode is not integrated > into Git GUI nor gitk, and it has been around for a while... How is it different from the "Color words" mode that sits in the drop down menu that defaults to "Line diff" ? https://imgur.com/a/qvo4oOC (Oh, I realize I had to draw the window wide to see it as it hides easily :/) > > Is there interest to incorporate this algorithm in the main git > > codebase? And if so, any hints on how to proceed? It depends on the the layering, look at xdiff/xdiffi.c xdl_do_diff() which has if (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF) return xdl_do_patience_diff(mf1, mf2, xpp, xe); if (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF) return xdl_do_histogram_diff(mf1, mf2, xpp, xe); as that produces the xdiff/ is for producing the diffs. All the coloring is at a later step, see diff.c (it's a big file), builtin_diff(), which uses fn_out_consume() as a callback from the xdiff/ stuff and assembles a git diff around the raw diff. diff.c is responsible for everything after the raw diff, including coloring, or getting the format of patches right (such as file names before a diff, matching up (renamed) files for diffing and such) > The best advice I have is to look at the `--color-words` mode. It comes > with its own "consume" function that accumulates lines from the diff, then > outputs them in a different way than the regular colored diff. Your mode > would want to do it very similarly. > > This is the accumulating part: > > https://github.com/git/git/blob/v2.19.0-rc1/diff.c#L1886 > > and this is the display part: > > https://github.com/git/git/blob/v2.19.0-rc1/diff.c#L2013 > > Basically, I would suggest to do a `git grep color.words` to find the > places where the `--color-words` mode is special-cased, and add new > special-casing for your mode. Which, BTW, I would suggest to find a > catchier name for ;-) AFAICT this is more than just a coloring scheme as it both produces a different low level diff, which would need code in the xdiff parts as well as colors, that is in diff.c. Thanks, Stefan
Re: [PATCH v2 06/10] push doc: correct lies about how push refspecs work
On Thu, Aug 30, 2018 at 5:23 PM Junio C Hamano wrote:
>
> Ævar Arnfjörð Bjarmason writes:
>
> > I.e. the non-refs/{tags,heads}/* update logic treats all updates to
> > tags/commits as branch updates. We just look at the tag v2.18.0, see you
> > want to replace it with the commit v2.19.0-rc0^{} and see "oh, that's a
> > fast-forward".
>
> In my old message you are responding to, I asked what you meant by
> "will be treated as branches", and after seeing "as branch updates"
> above, I think I know what you want the phrase to mean, namely, that
> old-to-new transition requires new to be a descendant of old. But I
> think that is weaker than what other people (including me) thinks of
> rules to update refs/heads/* hierarchy (i.e. "branch update").
>
> You are allowing to store an object that is not a commit in
> refs/blah/my-tag in your example, so it clearly does not protect the
> ref with an extra rule that applies to "branches", namely, "it has
> to be a commit."
Indeed. This was all confusing. I've reworded in something I'll send
shortly, which should address this confusion.
> > Arguably that should be changed, but I won't do that in this series.
>
> OK.
Re: A rebase regression in Git 2.18.0
On Tue, Aug 28, 2018 at 9:58 AM Junio C Hamano wrote: > > Elijah Newren writes: > > > - Add a flag to turn off directory rename detection, and set the > > flag for every call from am.c in order to avoid problems like this. > > I'd say this is the only practical solution, before you deprecate > the "pipe format-patch output to am -3" style of "git rebase" (and > optionally replace with something else). I posted a patch a while back to add an --am flag to "git rebase", make "--am" be implied by options which are still am-specific (--whitespace, --committer-date-is-author-date, and -C), and change --merge to be the default. I'll post it as an RFC again after the various rebase-rewrite series have settled and merged down...along with my other rebase cleanups that I was waiting on to avoid conflicts with GSoC stuff. > The whole point of "am -3" is to do _better_ than just "patch" with > minimum amount of information available on the pre- and post- image > blobs, without knowing the remainder of the tree that the patch did > not touch. It is not surprising that the heuristics that look at > the unchanging part of the tree to infer renames that may or may not > exist guesses incorrectly, either with false positive or negative. > In the context of "rebase", we always have all the trees that are > involved. We should be able to do better than "am -3".
Re: [PATCH 1/3] t3401: add another directory rename testcase for rebase and am
On Thu, Aug 30, 2018 at 9:01 AM Junio C Hamano wrote:
>
> Elijah Newren writes:
>
> >> Is this a single-shot environment assignment? That would have been
> >> caught with the test linter.
> >
> > Ugh, yes. Sorry.
> >
> > I was trying to allow backporting to 2.18, so tried to build my series
> > on that...but moved forward slightly to en/rebase-consistency in order
> > to re-use the same testfile (as mentioned in my cover letter). But
> > that meant my branch was missing a0a630192dca
> > ("t/check-non-portable-shell: detect "FOO=bar shell_func"",
> > 2018-07-13), so the test-linter couldn't catch this for me.
>
> True, I also only caught this during my integration cycle, not
> during the review of the patches.
>
> >> Perhaps squashing this in would be sufficient fix?
> >
> > Thanks for fixing it up. That works...although now I've spotted
> > another minor issue. The FAKE_LINES setting is only needed for the
> > interactive rebase case and can be removed for the other two. Oops.
> > It doesn't hurt, but might confuse readers of the testcase.
>
> Ah, OK. So the squashable fix would now become like this, all
> fixing the ones from the first patch.
>
> > Would you like me to resend a fixup on top of your fixup, resend the
> > whole series with the fixes squashed, or just ignore this final
> > (cosmetic) issue since it doesn't affect correctness and I've caused
> > you enough extra work already?
>
> No worries. This is what the maintainer does (when s/he is not
> playing other roles like a reviewer or an individual contributor).
>
> I'll squash in the following to the 1/3 patch and queue the topic
> with the other two patches.
Thanks, looks great.
> diff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh
> index 94bdfbd69c..e0b5111993 100755
> --- a/t/t3401-rebase-and-am-rename.sh
> +++ b/t/t3401-rebase-and-am-rename.sh
> @@ -141,7 +141,7 @@ test_expect_success 'rebase --interactive: NO directory
> rename' '
> git checkout B^0 &&
>
> set_fake_editor &&
> - FAKE_LINES="1" test_must_fail git rebase --interactive A &&
> + test_must_fail env FAKE_LINES="1" git rebase --interactive A
> &&
>
> git ls-files -s >out &&
> test_line_count = 6 out &&
> @@ -160,7 +160,7 @@ test_expect_success 'rebase (am): NO directory rename' '
> git checkout B^0 &&
>
> set_fake_editor &&
> - FAKE_LINES="1" test_must_fail git rebase A &&
> + test_must_fail git rebase A &&
>
> git ls-files -s >out &&
> test_line_count = 6 out &&
> @@ -179,7 +179,7 @@ test_expect_success 'rebase --merge: NO directory rename'
> '
> git checkout B^0 &&
>
> set_fake_editor &&
> - FAKE_LINES="1" test_must_fail git rebase --merge A &&
> + test_must_fail git rebase --merge A &&
>
> git ls-files -s >out &&
> test_line_count = 6 out &&
Re: What's cooking in git.git (Aug 2018, #06; Wed, 29)
On Thu, Aug 30, 2018 at 8:44 AM Junio C Hamano wrote: > > Elijah Newren writes: > > >> Recent addition of "directory rename" heuristics to the > >> merge-recursive backend makes the command susceptible to false > >> positives and false negatives, but the risk is even more grave when > >> used in the context of "git am -3", which does not know about any > >> surrounding unmodified paths while inspecting a patch. The > >> heuristic is disabled to keep the machinery "more stupid but > >> predicable". ... > > - Do we really want to say "even more" here? I'd rather we left those > > two words off or found another rewording. Obviously, I'm biased, but > > there's more than just my own opinion of and vested interest in the > > directory rename detection feature. I'm afraid users may interpret > > this sentence as saying the git project feels we've shipped a > > generally bad/unsafe feature, but are only taking corrective action in > > the most egregious of cases. That seems to me like a scary message to > > send. Maybe I'm just mis-reading what you meant, but I wanted to at > > least check what you meant here and, if that meaning was not > > intentional, ask whether we could improve the wording. > > I am biased towards "keep it stupid and simple and predictable" > camp, and want to make sure that users do not to blindly trust > overly-clever behaviour of the tool. As heuristics can always make > mistakes either way, I felt that not saying "more" would be sending > the opposite message---"in normal cases, dir-rename code will notice > presence or absense of whole-directory renames without mistakes but > when used in 'am -3' it misbehaves". > > But it was not my intention to say "it is generally bad/unsafe". I > just wanted to make sure that the users would understand it is "not > fool-proof and can make mistakes". > > Suggestions for a better rewrite is very much appreciated. That makes sense. I'm not sure I can concisely convey all the right points, but here's a stab at rewording: Recent addition of "directory rename" heuristics to the merge-recursive backend makes the command susceptible to false positives and false negatives. In the context of "git am -3", which does not know about surrounding unmodified paths and thus cannot inform the merge machinery about the full trees involved, this risk is particularly severe. As such, the heuristic is disabled for "git am -3" to keep the machinery "more stupid but predictable".
Re: [PATCH 1/3] t3401: add another directory rename testcase for rebase and am
Elijah Newren writes:
>> Is this a single-shot environment assignment? That would have been
>> caught with the test linter.
>
> Ugh, yes. Sorry.
>
> I was trying to allow backporting to 2.18, so tried to build my series
> on that...but moved forward slightly to en/rebase-consistency in order
> to re-use the same testfile (as mentioned in my cover letter). But
> that meant my branch was missing a0a630192dca
> ("t/check-non-portable-shell: detect "FOO=bar shell_func"",
> 2018-07-13), so the test-linter couldn't catch this for me.
True, I also only caught this during my integration cycle, not
during the review of the patches.
>> Perhaps squashing this in would be sufficient fix?
>
> Thanks for fixing it up. That works...although now I've spotted
> another minor issue. The FAKE_LINES setting is only needed for the
> interactive rebase case and can be removed for the other two. Oops.
> It doesn't hurt, but might confuse readers of the testcase.
Ah, OK. So the squashable fix would now become like this, all
fixing the ones from the first patch.
> Would you like me to resend a fixup on top of your fixup, resend the
> whole series with the fixes squashed, or just ignore this final
> (cosmetic) issue since it doesn't affect correctness and I've caused
> you enough extra work already?
No worries. This is what the maintainer does (when s/he is not
playing other roles like a reviewer or an individual contributor).
I'll squash in the following to the 1/3 patch and queue the topic
with the other two patches.
Thanks.
diff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh
index 94bdfbd69c..e0b5111993 100755
--- a/t/t3401-rebase-and-am-rename.sh
+++ b/t/t3401-rebase-and-am-rename.sh
@@ -141,7 +141,7 @@ test_expect_success 'rebase --interactive: NO directory
rename' '
git checkout B^0 &&
set_fake_editor &&
- FAKE_LINES="1" test_must_fail git rebase --interactive A &&
+ test_must_fail env FAKE_LINES="1" git rebase --interactive A &&
git ls-files -s >out &&
test_line_count = 6 out &&
@@ -160,7 +160,7 @@ test_expect_success 'rebase (am): NO directory rename' '
git checkout B^0 &&
set_fake_editor &&
- FAKE_LINES="1" test_must_fail git rebase A &&
+ test_must_fail git rebase A &&
git ls-files -s >out &&
test_line_count = 6 out &&
@@ -179,7 +179,7 @@ test_expect_success 'rebase --merge: NO directory rename' '
git checkout B^0 &&
set_fake_editor &&
- FAKE_LINES="1" test_must_fail git rebase --merge A &&
+ test_must_fail git rebase --merge A &&
git ls-files -s >out &&
test_line_count = 6 out &&
Re: What's cooking in git.git (Aug 2018, #06; Wed, 29)
Elijah Newren writes: >> Recent addition of "directory rename" heuristics to the >> merge-recursive backend makes the command susceptible to false >> positives and false negatives, but the risk is even more grave when >> used in the context of "git am -3", which does not know about any >> surrounding unmodified paths while inspecting a patch. The >> heuristic is disabled to keep the machinery "more stupid but >> predicable". > > I had separate comments about the SQUASH patch in the relevant thread, > but I've got a few comments on the release note itself, which I hope > are helpful: Yes indeed. > - Perhaps change the last sentence to '...heuristic is disabled for > "git am -3" to keep...', just to be slightly more clear about where it > is disabled? Yes, indeed that is a very good idea. > - Small spelling error: s/predicable/predictable/ This too. > - Do we really want to say "even more" here? I'd rather we left those > two words off or found another rewording. Obviously, I'm biased, but > there's more than just my own opinion of and vested interest in the > directory rename detection feature. I'm afraid users may interpret > this sentence as saying the git project feels we've shipped a > generally bad/unsafe feature, but are only taking corrective action in > the most egregious of cases. That seems to me like a scary message to > send. Maybe I'm just mis-reading what you meant, but I wanted to at > least check what you meant here and, if that meaning was not > intentional, ask whether we could improve the wording. I am biased towards "keep it stupid and simple and predictable" camp, and want to make sure that users do not to blindly trust overly-clever behaviour of the tool. As heuristics can always make mistakes either way, I felt that not saying "more" would be sending the opposite message---"in normal cases, dir-rename code will notice presence or absense of whole-directory renames without mistakes but when used in 'am -3' it misbehaves". But it was not my intention to say "it is generally bad/unsafe". I just wanted to make sure that the users would understand it is "not fool-proof and can make mistakes". Suggestions for a better rewrite is very much appreciated. Thanks.
Re: feature request: allow commit.email config setting
Rasmus Villemoes writes: > ... I can set GIT_COMMITTER_EMAIL in the environment, but that is > rather inconvenient, since that means I have to remember to do that in > the shell I'm using for that particular project, and I can't use that > shell for other projects. We only have user.email and user.name because nobody in the past 10+ years had such a requirement, but I find it it a perfectly sensible thing to wish to say "tagger.email and tagger.name are used while creating an annotated tag, committer.email and committer.name are used on the 'committer' line and author.email and author.name are used on the 'author' line in a newly created commit; by the way, if any of these are not set, but user.email or user.name is set, then they are used as fallback values." at the design level.
Re: [PATCH 0/1] v2.19.0-rc1 Performance Regression in 'git merge-base'
"Derrick Stolee via GitGitGadget" writes: > As I was testing the release candidate, I stumbled across a regression in > 'git merge-base' as a result of the switch to generation numbers. The commit > message in [PATCH 1/1] describes the topology involved,... I do not recall having seen this kind of updates during the pre-release freeze of previous development cycles, and you are starting a new trend with two findings so far in this cycle. Hopefully this can become a good tradition. Thanks.
Re: [PATCH 0/5] handle corruption in patch-delta
On Thu, 30 Aug 2018, Jeff King wrote: > On Wed, Aug 29, 2018 at 10:58:55PM +0200, Jann Horn wrote: > > > If `cmd` is in the range [0x01,0x7f] and `cmd > top-data`, the > > `memcpy(out, data, cmd)` can copy out-of-bounds data from after `delta_buf` > > into `dst_buf`. > > > > This is not an exploitable bug because triggering the bug increments the > > `data` pointer beyond `top`, causing the `data != top` sanity check after > > the loop to trigger and discard the destination buffer - which means that > > the result of the out-of-bounds read is never used for anything. > > > > Also, directly jump into the error handler instead of just breaking out of > > the loop - otherwise, data corruption would be silently ignored if the > > delta buffer ends with a command and the destination buffer is already > > full. > > Based on my earlier observations, here's a replacement patch series I > came up with. It has: > > [1/5]: test-delta: read input into a heap buffer > > A simpler replacement for your patch 2 which avoids portability > issues. > > [2/5]: t5303: test some corrupt deltas > > A more complete set of boundary tests based on the 4 cases I laid > out, plus the cp_size problem I found. > > [3/5]: patch-delta: fix oob read > > Your actual fix. > > [4/5]: patch-delta: consistently report corruption > > Your related trailing-garbage fix. I split this into two in order to > better demonstrate the cases this part covers. > > [5/5]: patch-delta: handle truncated copy parameters > > My fix for the cp_size read. > > I hope you don't mind me hacking up your patches a bit. Thanks again for > your original report and patch. Looks good to me (feels like traveling back in time). Reviewed-by: Nicolas Pitre > > -Peff >
Re: [PATCH v2 06/10] push doc: correct lies about how push refspecs work
Ævar Arnfjörð Bjarmason writes:
> I.e. the non-refs/{tags,heads}/* update logic treats all updates to
> tags/commits as branch updates. We just look at the tag v2.18.0, see you
> want to replace it with the commit v2.19.0-rc0^{} and see "oh, that's a
> fast-forward".
In my old message you are responding to, I asked what you meant by
"will be treated as branches", and after seeing "as branch updates"
above, I think I know what you want the phrase to mean, namely, that
old-to-new transition requires new to be a descendant of old. But I
think that is weaker than what other people (including me) thinks of
rules to update refs/heads/* hierarchy (i.e. "branch update").
You are allowing to store an object that is not a commit in
refs/blah/my-tag in your example, so it clearly does not protect the
ref with an extra rule that applies to "branches", namely, "it has
to be a commit."
> Arguably that should be changed, but I won't do that in this series.
OK.
Re: Automatic core.autocrlf?
On Wed, Aug 29, 2018 at 11:54 PM Jonathan Nieder wrote: > > Hi, > > Robert Dailey wrote: > > > Is there an 'auto' setting for the 'core.autocrlf' config? Reason I > > ask is, I want that setting to be 'input' on linux but 'true' on > > Windows. > > Others are exploring your question about the configuration language, > but I want to emphasize some other ramifications. > > Why do we still have 'core.autocrlf'? Do 'core.eol' and related > settings take care of that need, or is autocrlf still needed? If > core.eol etc do not take care of this need, what should we do to get > them to? > > Thanks, after having run into a few too many autocrlf-related messes, > Jonathan >From my perspective, the confusion is due to the evolution of the feature. There's multiple ways to control EOL handling but most of it is legacy/backward compatibility, I think. core.autocrlf is a fall-back for repos that do not have a .gitattributes. Because .gitattributes is optional by design, I'm not sure if getting rid of the config options is a good idea. But your point did make me think about how `core.autocrlf = true` should probably be a system config default for the Git for Windows project. The default for that value should be platform-defined. That would make it automatically work the way I want, and might solve a lot of the issues where people are committing CRLF into repositories on Windows.
Re: Automatic core.autocrlf?
On Mon, Aug 27, 2018 at 12:32 PM Andrei Rybak wrote: > > On 2018-08-27 17:52, Duy Nguyen wrote: > > On Mon, Aug 27, 2018 at 5:37 PM Torsten Bögershausen wrote: > >>> In those cases, when it falls back to > >>> configuration for line ending management, I want it to be > >>> automatically configured based on the host platform. > >> > > > > An alternative is supporting conditional config includes based on > > platform or host name, but I don't know if there are more use cases > > like this to justify it. > > > > How about just using unconditional includes? > > global.gitconfig (synced across machines): > > [include] > path = platform-specific.gitconfig > > And two version of file named "platform-specific.gitconfig", which > are not synced, and include only code.autocrlf setting. I think I tried this some years back, but ended up ditching it because when you modify settings via `git config --global`, it doesn't put values in the right files. This is probably the best answer so far though. It would still be great to have a mechanism that works within 1 file and is friendly to the git config command.
Re: [PATCH v2 06/10] push doc: correct lies about how push refspecs work
On Tue, Jul 31 2018, Junio C Hamano wrote:
I'm finally getting to re-rolling this. Just some inline comments.
> Ævar Arnfjörð Bjarmason writes:
>
>> The is often the name of the branch you would want to push, but
>> -it can be any arbitrary "SHA-1 expression", such as `master~4` or
>> -`HEAD` (see linkgit:gitrevisions[7]).
>> +it can be any arbitrary expression to a commit, such as `master~4` or
>> +`HEAD` (see linkgit:gitrevisions[7]). It can also refer to tag
>> +objects, trees or blobs if the is outside of `refs/heads/*`.
>
> "It can also refer to..." is a good addition, but do you really want
> to make it part of this series to change/deprecate "SHA-1 expression"
> (which would certainly involve discussion on "then what to call them
> instead, now we are trying to refrain from saying SHA-1?")?
I won't change that.
>> +on the remote side. Whether this is allowed depends on where in
>> +`refs/*` the reference lives. The `refs/heads/*` namespace will
>> +only accept commit objects, and then only they can be
>> +fast-forwarded. The `refs/tags/*` namespace will accept any kind of
>> +object, and any changes to them and others types of objects will be
>> +rejected. Finally, it's possible to push any type of object to any
>> +namespace outside of `refs/{tags,heads}/*`,
>
> All sound correct.
>
>> but these will be treated
>> +as branches for the purposes of whether `--force` is required, even in
>> +the case where a tag object is pushed.
>
> I am not sure what "will be treated as branches" exactly means.
> Does it mean "as if they were in refs/heads/* hierarchy?" Or
> something else?
I'll clarify this. Have rewritten most of this.
>> That tag object will be
>> +overwritten by another tag object (or commit!) without `--force` if
>> +the new tag happens to point to a commit that's a fast-forward of the
>> +commit it replaces.
>
> Yup, and that is something we want to fix with a later part of this
> series.
>
For what it's worth this is not at all what I'm fixing. The new docs
describe this better, but what I'm talking about here is that you can
push a tag like git.git's v2.18.0 to refs/blah/my-tag, then you can push
v2.19.0-rc0^{} to refs/blah/my-tag and it'll be allowed as a
fast-forward, and then v2.19.0-rc1 etc.
I.e. the non-refs/{tags,heads}/* update logic treats all updates to
tags/commits as branch updates. We just look at the tag v2.18.0, see you
want to replace it with the commit v2.19.0-rc0^{} and see "oh, that's a
fast-forward".
Arguably that should be changed, but I won't do that in this series.
>> +By having the optional leading `+` to a refspec (or using `--force`
>> +command line option) you can tell Git to update the ref even if
>> +it is not allowed by its respective namespace clobbering rules (e.g.,
>> +it is not a fast-forward. in the case of `refs/heads/*` updates).
>
> This gives an impression that with "--force" you can put non-commit
> inside refs/heads/* hierarchy. Is that correct (if so we probably
> would want to fix that behaviour)?
I'll fix the wording, but nope, luckily you can't do that.
>> +This
>> +does *not* attempt to merge into . See EXAMPLES below for
>> +details.
>
> That is not wrong per-se, but would normal people expect a merge to
> happen upon pushing on the other side, I wonder?
>
> Thanks for cleaning up our longstanding mess.
Will fix/reword.
Re: [PATCH] add -p: coalesce hunks before testing applicability
Phillip Wood writes:
> When $newhunk is created it is marked as dirty to prevent
> coalesce_overlapping_hunks() from coalescing it. This patch does not
> change that. What is happening is that by calling
> coalesce_overlapping_hunks() the hunks that are not currently selected
> are filtered out and any hunks that can be coalesced are (I think that
> in the test that starts passing with this patch the only change is the
> filtering as there's only a single hunk selected).
>
> This is a subtle change to the test for the applicability of an edited
> hunk. Previously when all the hunks were used to create the test patch
> we could be certain that if the test patch applied then if the user
> later selected any unselected hunk or deselected any selected hunk
> then that operation would succeed. I'm not sure that is true now (but
> I haven't thought about it for very long). We could restore the old
> test condition and coalesce the hunks by copying all the hunks and
> setting $hunk->{USE}=1 when creating the test patch if that turns out
> to be useful (it would be interesting to see if the test still passes
> with that change).
>
> Best Wishes
>
> Phillip
OK, I marked the topic as "will merge to next" but unmark it for
now, as we are not in a hurry to graduate new topics to 'master'
anyway. Hopefully between Jochen and you, perhaps others, can
explore the issues you raised and come to some conclusion before it
becomes necessary (i.e. when the next cycle begins).
Thanks.
Re: Feature request: hooks directory
On Thu, Aug 30 2018, Wesley Schwengle wrote: > Hello all, > > I would like to ask if it is worth my time looking into the following > solution to a problem we have at work. > > Problem: > We want to have some git-hooks and we want to provide them to the > user. In a most recent example we have a post-checkout hook that deals > with some Docker things. However, if we update that post-checkout hook > my local overrides in that post-checkout hook are going to be > overwritten. > > Solution: > We discussed this at work and we thought about making a .d directory > for the hooks, eg. $GIT_DIR/hooks/post-commit.d, where a user can put > the post-commit hooks in. This allows us to provide post commit hooks > and allows the user to add additional hooks him/herself. We could > implement this in our own code base. But we were wondering if this > approach could be shared with the git community and if this behavior > is wanted in git itself. There is interest in this. This E-Mail of mine gives a good summary of prior discussions about this: https://public-inbox.org/git/877eqqnq22@evledraar.gmail.com/ I.e. it's something I've personally been interested in doing in the past, there's various bolt-on solutions to do it (basically local hook runners) used by various projects.
OFFRE DE PRÊT ENTRE PARTICULIER SÉRIEUX ET FIABLE ...
Bonjour Vous aviez besoin de prêts d'argent entre particuliers pour faire face aux difficultés financières pour enfin sortir de l'impasse que provoquent les banques, par le rejet de vos dossiers de demande de crédits ? Je suis Pierre ARNOUX, citoyen français, et gestionnaire de Portefeuille de la famille LACASSIN. Par moi, cette famille disposant d'un énorme capital, peut vous prêter de 5000 euros à 80€ . Voici les domaines dans lesquels je peux vous aider à obtenir un prêt auprès de la famille LACASSIN dont je suis le gestionnaire de portefeuille : * Prêt a la consommation * Prêt immobilier * Prêt à l'investissement * Prêt automobile * Dette de consolidation * Marge de crédit * Deuxième hypothèque * Rachat de crédit * Prêt personnel * Prêt Etudiant. Vous êtes fichés, interdits bancaires et vous n'avez pas la faveur des banques ou mieux vous avez un projet et besoin de financement, un mauvais dossier de crédit ou besoin d'argent pour payer des factures, fonds à investir dans les entreprises. Alors N’hésitez pas a me recontacter a l'adresse pierrearnou...@yahoo.com avec votre demande si vous êtes intéressé. NB : Pas sérieux s’abstenir. Merci Pierre ARNOUX
Re: Trivial enhancement: All commands which require an author should accept --author
Ævar Arnfjörð Bjarmason writes: > I believe the "official" way, such as it is, is you just put > #leftoverbits in your E-Mail, then search the list archives, > e.g. https://public-inbox.org/git/?q=%23leftoverbits I think that technique has been around long enough to be called a recognised way, but I do not think it is "the" official way. It is one of the efforts to allow us remember what we might want to work on, and focuses on not wasting too much efforts in curating. Another effort to allow us remember is http://crbug.com/git that is run by Jonathan Nieder. Anybody can participate in curating the latter. The former is uncurated and deliberately kept informal, but will stay a usable way until clueless people catch up with the practice and mark any random garbage they come up with with the marking word. I myself try to refrain from using it when I raise the idea/issue for the first time to avoid "ah, it turns out that it is not such a great idea after thinking about it for a while"--rather I try to limit my use to my responses as a reaction to somebody else's idea/issue. That way, I can make sure that messages with the marking word from me has idea supported by at least two people, one of which is known to me to have a good taste, so mailing list search "from:me #leftoverbits" would stay meaningful.
Re: Trivial enhancement: All commands which require an author should accept --author
Hi Ævar, On Thu, 30 Aug 2018, Ævar Arnfjörð Bjarmason wrote: > On Thu, Aug 30 2018, Johannes Schindelin wrote: > > > On Wed, 29 Aug 2018, Junio C Hamano wrote: > > > >> Johannes Schindelin writes: > >> > >> > The `stash` command only incidentally requires that the author is set, as > >> > it calls `git commit` internally (which records the author). As stashes > >> > are intended to be local only, that author information was never meant to > >> > be a vital part of the `stash`. > >> > > >> > I could imagine that an even better enhancement request would ask for > >> > `git > >> > stash` to work even if `user.name` is not configured. > >> > >> This would make a good bite-sized microproject, worth marking it as > >> #leftoverbits unless somebody is already working on it ;-) > > > > Right. > > > > What is our currently-favored approach to this, again? Do we have a > > favorite wiki page to list those, or do we have a bug tracker for such > > mini-projects? > > > > Once I know, I will add this, with enough information to get anybody > > interested started. > > I believe the "official" way, such as it is, is you just put > #leftoverbits in your E-Mail, then search the list archives, > e.g. https://public-inbox.org/git/?q=%23leftoverbits > > So e.g. I've taken to putting this in my own E-Mails where I spot > something I'd like to note as a TODO that I (or someone else) could work > on later: > https://public-inbox.org/git/?q=%23leftoverbits+f%3Aavarab%40gmail.com That is a poor way to list the current micro-projects, as it is totally non-obvious to the casual interested person which projects are still relevant, and which ones have been addressed already. In a bug tracker, you can at least add a comment stating that something has been addressed, or made a lot easier by another topic. In a mailing list archive, those mails are immutable, and you cannot update squat. Ciao, Johannes
es/format-patch-{inter,range}diff, was Re: What's cooking in git.git (Aug 2018, #06; Wed, 29)
Hi Junio,
On Wed, 29 Aug 2018, Junio C Hamano wrote:
> * es/format-patch-interdiff (2018-07-23) 6 commits
> - format-patch: allow --interdiff to apply to a lone-patch
> - log-tree: show_log: make commentary block delimiting reusable
> - interdiff: teach show_interdiff() to indent interdiff
> - format-patch: teach --interdiff to respect -v/--reroll-count
> - format-patch: add --interdiff option to embed diff in cover letter
> - format-patch: allow additional generated content in make_cover_letter()
> (this branch is used by es/format-patch-rangediff.)
>
> "git format-patch" learned a new "--interdiff" option to explain
> the difference between this version and the previous atttempt in
> the cover letter (or after the tree-dashes as a comment).
>
> What's the doneness of this one?
> cf.
I looked over the changes online, and I think they are good.
The only slightly iffy thing I found was using the function parameter
`rerolled` as printf-style format in
https://github.com/gitgitgadget/git/compare/es/format-patch-interdiff~6...es/format-patch-interdiff#diff-71d4a6bddc3e479f9abf11900878a0b2R1430:
static const char *diff_title(struct strbuf *sb, int reroll_count,
const char *generic, const char *rerolled)
{
if (reroll_count <= 0)
strbuf_addstr(sb, generic);
else /* RFC may be v0, so allow -v1 to diff against v0 */
strbuf_addf(sb, rerolled, reroll_count - 1);
return sb->buf;
}
I guess that's okay, though. (I would have done it differently, but that
would have meant playing sentence lego with the "Interdiff against v%d:"
string.)
> * es/format-patch-rangediff (2018-08-14) 10 commits
> - format-patch: allow --range-diff to apply to a lone-patch
> - format-patch: add --creation-factor tweak for --range-diff
> - format-patch: teach --range-diff to respect -v/--reroll-count
> - format-patch: extend --range-diff to accept revision range
> - format-patch: add --range-diff option to embed diff in cover letter
> - range-diff: relieve callers of low-level configuration burden
> - range-diff: publish default creation factor
> - range-diff: respect diff_option.file rather than assuming 'stdout'
> - Merge branch 'es/format-patch-interdiff' into es/format-patch-rangediff
> - Merge branch 'js/range-diff' into es/format-patch-rangediff
> (this branch uses es/format-patch-interdiff.)
>
> "git format-patch" learned a new "--range-diff" option to explain
> the difference between this version and the previous atttempt in
> the cover letter (or after the tree-dashes as a comment).
>
> What's the doneness of this one?
I just had a look at the diff online, and I think this is ready for next.
Personally, I would have put the infer_range_diff_ranges() function
(https://github.com/gitgitgadget/git/compare/es/format-patch-rangediff~8...es/format-patch-rangediff#diff-71d4a6bddc3e479f9abf11900878a0b2R1448)
into `range-diff.c`, but it is too minor a thing to ask for a new patch
series iteration.
It also looks slightly murky to me that `show_range_diff()` is now using
a copy of the `diffopts` (see
https://github.com/gitgitgadget/git/compare/es/format-patch-rangediff~8...es/format-patch-rangediff#diff-ab6f5eca48b8e84edf999acbe3fe7553R435),
but I have no idea how to do this in a more elegant manner, either.
In short: from my point of view, both topics are ready for `next`.
Ciao,
Dscho
Re: [PATCH] add -p: coalesce hunks before testing applicability
Dear Jochen/Junio
On 28/08/18 19:07, Junio C Hamano wrote:
Jochen Sprickerhof writes:
When a hunk was split before being edited manually, it does not apply
anymore cleanly. Apply coalesce_overlapping_hunks() first to make it
work. Enable test for it as well.
Signed-off-by: Jochen Sprickerhof
---
git-add--interactive.perl | 8
t/t3701-add-interactive.sh | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 36f38ced9..c9f434e4a 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1195,10 +1195,10 @@ sub edit_hunk_loop {
# delta from the original unedited hunk.
$hunk->{OFS_DELTA} and
$newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA};
- if (diff_applies($head,
-@{$hunks}[0..$ix-1],
-$newhunk,
-@{$hunks}[$ix+1..$#{$hunks}])) {
+ my @hunk = @{$hunks};
+ splice (@hunk, $ix, 1, $newhunk);
+ @hunk = coalesce_overlapping_hunks(@hunk);
+ if (diff_applies($head, @hunk)) {
$newhunk->{DISPLAY} = [color_diff(@{$newtext})];
return $newhunk;
}
OK. I think I understand how this may be needed in certain forms of
edit.
When $newhunk is created it is marked as dirty to prevent
coalesce_overlapping_hunks() from coalescing it. This patch does not
change that. What is happening is that by calling
coalesce_overlapping_hunks() the hunks that are not currently selected
are filtered out and any hunks that can be coalesced are (I think that
in the test that starts passing with this patch the only change is the
filtering as there's only a single hunk selected).
This is a subtle change to the test for the applicability of an edited
hunk. Previously when all the hunks were used to create the test patch
we could be certain that if the test patch applied then if the user
later selected any unselected hunk or deselected any selected hunk then
that operation would succeed. I'm not sure that is true now (but I
haven't thought about it for very long). We could restore the old test
condition and coalesce the hunks by copying all the hunks and setting
$hunk->{USE}=1 when creating the test patch if that turns out to be
useful (it would be interesting to see if the test still passes with
that change).
Best Wishes
Phillip
I do not think coalesce would reliably work against arbitrary
kind of edit, but the function is called at the end of the loop of
the caller of this function anyway, so it is not like this is making
anything worse at all. Let's queue it and try it out.
Thanks.
All of the following is a tangent for future/further work, and
should not be done as part of your patch. While this change may
work around the immediate issue of diff_applies() returning "no", it
makes it feel a bit iffy to keep the interface to return $newhunk.
With the current interface, the function is saying the caller "here
is a text that shows a new hunk, when spliced back into the @hunk
array and coalesced together with others, would apply cleanly to the
current index". But the corrected code is already doing a trial
splice with trial coalescing anyway, so perhaps it may make more
sense to update the interface into this function for the caller to
say "the user asks to edit $ix'th hunk in @$hunks" and the function
to answer "Here is the edited result in @$hunks; I've made sure it
would cleanly apply".
Incidentally, that would make it possible in the future to allow
edit_hunk_loop to be told "the user wants to edit this $ix'th hunk",
allow the editor to split that hunk into multiple hunks, and return
the result by stuffing them (not just a single $newhunk) into the
@hunk array. The caller's loop could then further select or join
these hunks---such an interaction is impossible with the current
interface that allows edit_hunk_loop to take a single hunk and
return a single newhunk.
But again, all of the above is a tangent for future/further work,
and should not be done as part of your patch.
Re: [PATCH 0/5] handle corruption in patch-delta
On Thu, Aug 30, 2018 at 9:05 AM Jeff King wrote: > > On Wed, Aug 29, 2018 at 10:58:55PM +0200, Jann Horn wrote: > > > If `cmd` is in the range [0x01,0x7f] and `cmd > top-data`, the > > `memcpy(out, data, cmd)` can copy out-of-bounds data from after `delta_buf` > > into `dst_buf`. > > > > This is not an exploitable bug because triggering the bug increments the > > `data` pointer beyond `top`, causing the `data != top` sanity check after > > the loop to trigger and discard the destination buffer - which means that > > the result of the out-of-bounds read is never used for anything. > > > > Also, directly jump into the error handler instead of just breaking out of > > the loop - otherwise, data corruption would be silently ignored if the > > delta buffer ends with a command and the destination buffer is already > > full. > > Based on my earlier observations, here's a replacement patch series I > came up with. It has: [...] > I hope you don't mind me hacking up your patches a bit. Not at all. I'm happy that I don't have to write a v2 series. > Thanks again for your original report and patch. Thanks for turning my patch into something decent so quickly!
Feature request: hooks directory
Hello all, I would like to ask if it is worth my time looking into the following solution to a problem we have at work. Problem: We want to have some git-hooks and we want to provide them to the user. In a most recent example we have a post-checkout hook that deals with some Docker things. However, if we update that post-checkout hook my local overrides in that post-checkout hook are going to be overwritten. Solution: We discussed this at work and we thought about making a .d directory for the hooks, eg. $GIT_DIR/hooks/post-commit.d, where a user can put the post-commit hooks in. This allows us to provide post commit hooks and allows the user to add additional hooks him/herself. We could implement this in our own code base. But we were wondering if this approach could be shared with the git community and if this behavior is wanted in git itself. Cheers, Wesley -- Wesley Schwengle, Developer Mintlab B.V., https://www.zaaksysteem.nl E: wes...@mintlab.nl T: +31 20 737 00 05
[PATCH 1/1] commit: don't use generation numbers if not needed
From: Derrick Stolee
In 3afc679b "commit: use generations in paint_down_to_common()",
the queue in paint_down_to_common() was changed to use a priority
order based on generation number before commit date. This served
two purposes:
1. When generation numbers are present, the walk guarantees
correct topological relationships, regardless of clock skew in
commit dates.
2. It enables short-circuiting the walk when the min_generation
parameter is added in d7c1ec3e "commit: add short-circuit to
paint_down_to_common()". This short-circuit helps commands
like 'git branch --contains' from needing to walk to a merge
base when we know the result is false.
The commit message for 3afc679b includes the following sentence:
This change does not affect the number of commits that are
walked during the execution of paint_down_to_common(), only
the order that those commits are inspected.
This statement is incorrect. Because it changes the order in which
the commits are inspected, it changes the order they are added to
the queue, and hence can change the number of loops before the
queue_has_nonstale() method returns true.
This change makes a concrete difference depending on the topology
of the commit graph. For instance, computing the merge-base between
consecutive versions of the Linux kernel has no effect for versions
after v4.9, but 'git merge-base v4.8 v4.9' presents a performance
regression:
v2.18.0: 0.122s
v2.19.0-rc1: 0.547s
HEAD: 0.127s
To determine that this was simply an ordering issue, I inserted
a counter within the while loop of paint_down_to_common() and
found that the loop runs 167,468 times in v2.18.0 and 635,579
times in v2.19.0-rc1.
The topology of this case can be described in a simplified way
here:
v4.9
| \
| \
v4.8 \
| \ \
| \ |
... A B
| / /
| / /
|/__/
C
Here, the "..." means "a very long line of commits". By generation
number, A and B have generation one more than C. However, A and B
have commit date higher than most of the commits reachable from
v4.8. When the walk reaches v4.8, we realize that it has PARENT1
and PARENT2 flags, so everything it can reach is marked as STALE,
including A. B has only the PARENT1 flag, so is not STALE.
When paint_down_to_common() is run using
compare_commits_by_commit_date, A and B are removed from the queue
early and C is inserted into the queue. At this point, C and the
rest of the queue entries are marked as STALE. The loop then
terminates.
When paint_down_to_common() is run using
compare_commits_by_gen_then_commit_date, B is removed from the
queue only after the many commits reachable from v4.8 are explored.
This causes the loop to run longer. The reason for this regression
is simple: the queue order is intended to not explore a commit
until everything that _could_ reach that commit is explored. From
the information gathered by the original ordering, we have no
guarantee that there is not a commit D reachable from v4.8 that
can also reach B. We gained absolute correctness in exchange for
a performance regression.
The performance regression is probably the worse option, since
these incorrect results in paint_down_to_common() are rare. The
topology required for the performance regression are less rare,
but still require multiple merge commits where the parents differ
greatly in generation number. In our example above, the commit A
is as important as the commit B to demonstrate the problem, since
otherwise the commit C will sit in the queue as non-stale just as
long in both orders.
The solution provided uses the min_generation parameter to decide
if we should use generation numbers in our ordering. When
min_generation is equal to zero, it means that the caller has no
known cutoff for the walk, so we should rely on our commit-date
heuristic as before; this is the case with merge_bases_many().
When min_generation is non-zero, then the caller knows a valuable
cutoff for the short-circuit mechanism; this is the case with
remove_redundant() and in_merge_bases_many().
Signed-off-by: Derrick Stolee
---
commit.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/commit.c b/commit.c
index 1a6e632185..449c1f4920 100644
--- a/commit.c
+++ b/commit.c
@@ -874,6 +874,9 @@ static struct commit_list *paint_down_to_common(struct
commit *one, int n,
int i;
uint32_t last_gen = GENERATION_NUMBER_INFINITY;
+ if (!min_generation)
+ queue.compare = compare_commits_by_commit_date;
+
one->object.flags |= PARENT1;
if (!n) {
commit_list_append(one, );
@@ -891,7 +894,7 @@ static struct commit_list *paint_down_to_common(struct
commit *one, int n,
struct commit_list *parents;
int flags;
- if (commit->generation > last_gen)
+ if (min_generation && commit->generation > last_gen)
BUG("bad generation skip %8x > %8x at
[PATCH 0/1] v2.19.0-rc1 Performance Regression in 'git merge-base'
As I was testing the release candidate, I stumbled across a regression in 'git merge-base' as a result of the switch to generation numbers. The commit message in [PATCH 1/1] describes the topology involved, but you can test it yourself by comparing 'git merge-base v4.8 v4.9' in the Linux kernel. The regression does not show up when running merge-base for tags at least v4.9, which is why I didn't see it when I was testing earlier. The solution is simple, but also will conflict with ds/reachable in next. I can send a similar patch that applies the same diff into commit-reach.c. With the integration of generation numbers into most commit walks coming to a close [1], it will be time to re-investigate other options for reachability indexes [2]. As I was digging into the issue with this regression, I discovered a way we can modify our generation numbers and pair them with commit dates to give us a simple-to-compute, immutable two-dimensional reachability index that would be immune to this regression. I will investigate that more and report back, but it is more important to fix this regression now. Thanks, -Stolee [1] https://public-inbox.org/git/pull.25.git.gitgitgad...@gmail.com/[PATCH 0/6] Use generation numbers for --topo-order [2] https://public-inbox.org/git/86muxcuyod@gmail.com/[RFC] Other chunks for commit-graph, part 2 - reachability indexes Cc: gitster@pobox.comCc: p...@peff.net Derrick Stolee (1): commit: don't use generation numbers if not needed commit.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) base-commit: 2f743933341f27603550fbf383a34dfcfd38 Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-28%2Fderrickstolee%2Fmerge-base-regression-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-28/derrickstolee/merge-base-regression-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/28 -- gitgitgadget
Re: Trivial enhancement: All commands which require an author should accept --author
On Thu, Aug 30 2018, Johannes Schindelin wrote: > Hi Junio, > > On Wed, 29 Aug 2018, Junio C Hamano wrote: > >> Johannes Schindelin writes: >> >> > The `stash` command only incidentally requires that the author is set, as >> > it calls `git commit` internally (which records the author). As stashes >> > are intended to be local only, that author information was never meant to >> > be a vital part of the `stash`. >> > >> > I could imagine that an even better enhancement request would ask for `git >> > stash` to work even if `user.name` is not configured. >> >> This would make a good bite-sized microproject, worth marking it as >> #leftoverbits unless somebody is already working on it ;-) > > Right. > > What is our currently-favored approach to this, again? Do we have a > favorite wiki page to list those, or do we have a bug tracker for such > mini-projects? > > Once I know, I will add this, with enough information to get anybody > interested started. I believe the "official" way, such as it is, is you just put #leftoverbits in your E-Mail, then search the list archives, e.g. https://public-inbox.org/git/?q=%23leftoverbits So e.g. I've taken to putting this in my own E-Mails where I spot something I'd like to note as a TODO that I (or someone else) could work on later: https://public-inbox.org/git/?q=%23leftoverbits+f%3Aavarab%40gmail.com
Re: Git in Outreachy Dec-Mar?
On Thu, Aug 30 2018, Jeff King wrote: > On Wed, Aug 29, 2018 at 03:12:37PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> > 2. To get our landing page and list of projects in order (and also >> > micro-projects for applicants). This can probably build on the >> > previous round at: >> > >> >https://git.github.io/Outreachy-15/ >> > >> > and on the project/microprojects lists for GSoC (which will need >> > some updating and culling). >> [...] >> I just have a "yes" to the first one of those. Which tells you how much >> skin I have in the game (and how much you should(n't) listen to me) :) > > Yes, if nobody steps up to do 2, then it won't happen. :) > > For myself, I don't think I have time to commit to mentoring this round. > And IMHO the people signing up to mentor should be the ones contributing > to the project list (since they will ultimately be on the hook for > working on those projects with the intern). > >> Just a question: It seems to me that #1 and #2 is not tied up to the >> Outreachy process. I agree that finding a qualified intern to work on >> Git would be a good use of project funds. >> >> What's not clear to me is if/how tied up this needs to be to a specific >> external program such as Outreachy. I.e. do we as a project need to go >> through that organization, or can that be just one of the ways in which >> we send out a call for interns? Thanks! > It doesn't need to be. As far as I know, the main reasons (from the > perspective of a project) to do it through Outreachy are: > > - being part of a larger program generates attention and gets the >interest of intern candidates (free advertising, if you will) I was wondering if we couldn't do it through Outreachy *and* also do our own advertisements / possibly recruit candidates outside of the Outreachy pool. In that case we'd still get the attention/outreach benefits, in addition to our own... > - Outreachy handles payment, invoicing for external funds, and any >legal stuff > > - it's possibly easier to external funding if it's earmarked for a >program like Outreachy, since that program provides a framework with >particular goals, conditions, oversight, etc. ... but not both of these at least if we selected any a non-Outreachy candidates. Nice to get a good summary of the pros there. > I think there's some general value in having a group, too. Because > there are many interns all participating at the same time, they can > offer each other support or advice, show off their work to each other > via blog posts, etc. And it may be easier for them to communicate > about their accomplishments and status for future work, since it's > part of an established program that can easily be explained. Yup, but just as a clarifying point here wouldn't the participants also get all the same benefits of this in the case of Outreachy+OurOwnProgram if we ran OurOwnProgram concurrently to Outreachy? I.e. I was assuming that once candidates are "handed off" to a project they're communicating within that project (possibly with other candidates), and Outreachy is no longer very involved (except maybe for progress reports / final report, but wouldn't we also do that for a OurOwnProgram?). I may have that completely wrong though, which is why I'm asking, which b.t.w. I'm doing mostly just to get an idea of how what Outreachy's role is in this exactly, not to strongly advocate for a OurOwnProgram. > As for reasons _not_ to do it, I don't think the requirements are particularly > onerous. Mostly it's: > > - it has to happen at a specific time, which might not be convenient > for mentors or interns (last year I found it hard to get focused > starting in December, with all of the holidays) Yup. > - it naturally limits the candidate pool to under-represented groups > (which is the whole point of the program, but if you don't > actually care about that, then it's just a complication) I'm fine with doing selection discrimination of under-represented groups through such a program. Particularly if, as you mention, there's earmarked funding for it which otherwise might not be available, so it's not zero-sum when it comes to a hypothetical alternative of casting a wider net of our own (and as you mention, that would be more work). I do think it's unfortunate that the selection criteria for the program privileges U.S. citizens and U.S. residents above other people, particularly since they're also accepting worldwide candidates (and we've had at least one non-American participant that I know about), so it's not e.g. for U.S. administrative or tax reasons as one might expect if they only accepted Americans. I don't think that's some big deal, just something that puts the Git project as an international cooperation (and one that solicits funds from donors worldwide) into a slightly odd position, so something we should keep in mind going forward. > So IMHO it's easily worth the trouble. > >> With GSoC
