* Re: [PATCH 2/3] get_worktrees() must return main worktree as first item even on error
From: Junio C Hamano @ 2016-11-23 17:06 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, rappazzo
In-Reply-To: <20161122100046.8341-3-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 5c4854d..b835b91 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -388,7 +388,7 @@ static void show_worktree_porcelain(struct worktree *wt)
> printf("HEAD %s\n", sha1_to_hex(wt->head_sha1));
> if (wt->is_detached)
> printf("detached\n");
> - else
> + else if (wt->head_ref)
> printf("branch %s\n", wt->head_ref);
This change looks somewhat unrelated to what the title and the log
message claims to do, but the fix is to indicate an error condition
by leaving wt->head_ref as NULL, so this is a necessary adjustment.
Good.
> @@ -406,10 +406,12 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
> else {
> strbuf_addf(&sb, "%-*s ", abbrev_len,
> find_unique_abbrev(wt->head_sha1, DEFAULT_ABBREV));
> - if (!wt->is_detached)
> + if (wt->is_detached)
> + strbuf_addstr(&sb, "(detached HEAD)");
> + else if (wt->head_ref)
> strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(wt->head_ref, 0));
> else
> - strbuf_addstr(&sb, "(detached HEAD)");
> + strbuf_addstr(&sb, "(error)");
> }
Likewise.
> diff --git a/worktree.c b/worktree.c
> index f7c1b5e..a674efa 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -89,7 +89,7 @@ static struct worktree *get_main_worktree(void)
> strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
>
> if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
> - goto done;
> + strbuf_reset(&head_ref);
>
> worktree = xcalloc(1, sizeof(*worktree));
> worktree->path = strbuf_detach(&worktree_path, NULL);
> @@ -97,7 +97,6 @@ static struct worktree *get_main_worktree(void)
> worktree->is_detached = is_detached;
> add_head_info(&head_ref, worktree);
OK. The earlier call to _reset() and add_head_info() function
itself may want to be clarified that a zero-length strbuf signals an
error condition with additional comment. It is all too unclear in
the code with this patch as it stands.
> -done:
> strbuf_release(&path);
> strbuf_release(&worktree_path);
> strbuf_release(&head_ref);
After this there is "return worktree" which used to return NULL
because of the "goto", but we never return NULL from the function
after this change, which is the whole point of this change. Good.
> @@ -173,8 +172,7 @@ struct worktree **get_worktrees(void)
>
> list = xmalloc(alloc * sizeof(struct worktree *));
>
> - if ((list[counter] = get_main_worktree()))
> - counter++;
> + list[counter++] = get_main_worktree();
Hence the conditional, while it does not hurt, becomes unnecessary
and we can unconditionally throw the primary one to the list.
Good.
Other than the "these need in-code commenting", and also that this
should have a new test, the patch makes sense to me.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/3] worktree.c: zero new 'struct worktree' on allocation
From: Junio C Hamano @ 2016-11-23 16:52 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, rappazzo
In-Reply-To: <20161122100046.8341-2-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> This keeps things a bit simpler when we add more fields, knowing that
> default values are always zero.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
Looks sensible. Thanks.
> worktree.c | 14 ++------------
> 1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/worktree.c b/worktree.c
> index f7869f8..f7c1b5e 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -91,16 +91,11 @@ static struct worktree *get_main_worktree(void)
> if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
> goto done;
>
> - worktree = xmalloc(sizeof(struct worktree));
> + worktree = xcalloc(1, sizeof(*worktree));
> worktree->path = strbuf_detach(&worktree_path, NULL);
> - worktree->id = NULL;
> worktree->is_bare = is_bare;
> - worktree->head_ref = NULL;
> worktree->is_detached = is_detached;
> - worktree->is_current = 0;
> add_head_info(&head_ref, worktree);
> - worktree->lock_reason = NULL;
> - worktree->lock_reason_valid = 0;
>
> done:
> strbuf_release(&path);
> @@ -138,16 +133,11 @@ static struct worktree *get_linked_worktree(const char *id)
> if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
> goto done;
>
> - worktree = xmalloc(sizeof(struct worktree));
> + worktree = xcalloc(1, sizeof(*worktree));
> worktree->path = strbuf_detach(&worktree_path, NULL);
> worktree->id = xstrdup(id);
> - worktree->is_bare = 0;
> - worktree->head_ref = NULL;
> worktree->is_detached = is_detached;
> - worktree->is_current = 0;
> add_head_info(&head_ref, worktree);
> - worktree->lock_reason = NULL;
> - worktree->lock_reason_valid = 0;
>
> done:
> strbuf_release(&path);
^ permalink raw reply
* Re: [PATCH 0/3] Minor fixes on 'git worktree'
From: Junio C Hamano @ 2016-11-23 16:52 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, rappazzo
In-Reply-To: <20161122100046.8341-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> This fixes two things:
>
> - make sure the first item is always the main worktree even if we
> fail to retrieve some info
>
> - keep 'worktree list' order stable (which in turn fixes the random
> failure on my 'worktree-move' series
> Nguyễn Thái Ngọc Duy (3):
> worktree.c: zero new 'struct worktree' on allocation
> get_worktrees() must return main worktree as first item even on error
> worktree list: keep the list sorted
>
> builtin/worktree.c | 26 ++++++++++++++++++++++----
> worktree.c | 20 ++++----------------
> 2 files changed, 26 insertions(+), 20 deletions(-)
Any tests?
^ permalink raw reply
* Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules
From: Junio C Hamano @ 2016-11-23 16:51 UTC (permalink / raw)
To: Brandon Williams
Cc: Stefan Beller, git@vger.kernel.org, Heiko Voigt, Johannes Sixt
In-Reply-To: <20161122181839.GF149321@google.com>
Brandon Williams <bmwill@google.com> writes:
> On 11/22, Junio C Hamano wrote:
>> Brandon Williams <bmwill@google.com> writes:
>>
>> > On 11/17, Stefan Beller wrote:
>> >> On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams <bmwill@google.com> wrote:
>> >>
>> >> > sha1_array_clear(&commits);
>> >> > - die("Failed to push all needed submodules!");
>> >> > + die ("Failed to push all needed submodules!");
>> >>
>> >> huh? Is this a whitespace change?
>> >
>> > That's odd...I didn't mean to add that lone space.
>>
>> Is that the only glitch in this round? IOW, is the series OK to be
>> picked up as long as I treak this out while queuing?
>
> It looks that way. And I did fix this in my local series. Let me know
> if you would rather I resend the series. Otherwise I think it looks
> good.
OK, queued with trivial fix for now.
Thanks.
^ permalink raw reply
* Re: [PATCH v1 15/19] config: add git_config_get_date_string() from gc.c
From: Christian Couder @ 2016-11-23 15:04 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <xmqqziljngod.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 1, 2016 at 8:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> This function will be used in a following commit to get the expiration
>> time of the shared index files from the config, and it is generic
>> enough to be put in "config.c".
>
> Is it generic enough that a helper that sounds as if it can get any
> date string dies if it is given a future date? I somehow doubt it.
>
> At the minimum, it must be made clear that there is an artificial
> limitation that the current set of callers find useful in cache.h as
> a one-liner comment next to the added declaration. Then people with
> the same need (i.e. they want to reject future timestamps) can
> decide to use it, while others would stay away from it.
>
> If you can come up with a better word to use to encode that
> artificial limitation in its name, renaming it is even better.
Ok it will appear like this in cache.h:
/* This dies if the configured or default date is in the future */
extern int git_config_get_expire_date_string(const char *key, const
char **output);
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH 2/2] difftool: add a feature flag for the builtin vs scripted version
From: Dennis Kaarsemaker @ 2016-11-23 14:51 UTC (permalink / raw)
To: Johannes Schindelin, git; +Cc: Junio C Hamano
In-Reply-To: <598dcfdbeef4e15d2d439053a0423589182e5f30.1479834051.git.johannes.schindelin@gmx.de>
On Tue, 2016-11-22 at 18:01 +0100, Johannes Schindelin wrote:
> The original idea was to use an environment variable
> GIT_USE_BUILTIN_DIFFTOOL, but the test suite resets those variables, and
> we do want to use that feature flag to run the tests with, and without,
> the feature flag.
>
> Besides, the plan is to add an opt-in flag in Git for Windows'
> installer. If we implemented the feature flag as an environment
> variable, we would have to modify the user's environment, in order to
> make the builtin difftool the default when called from Git Bash, Git CMD
> or third-party tools.
Hi Johannes,
Why is this not a normal configuration variable (as in git config
difftool.builtin true or something)? It doesn't make much sense to me
to introduce a way of configuring git by introducing magic files, when
a normal configuration variable would do just fine, and the GfW
installer can also set such variables, like it does for the crlf config
I believe.
--
Dennis Kaarsemaker
http://www.kaarsemaker.net
^ permalink raw reply
* Re: dangling commits in worktree
From: Luc Van Oostenryck @ 2016-11-23 14:15 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8CPX3PDfhcaftDHy_U37rEACr7Q1gj_un4ALen45J9GZQ@mail.gmail.com>
On Wed, Nov 23, 2016 at 04:45:56PM +0700, Duy Nguyen wrote:
>
> It's a known issue that gc (and maybe some others that do rev-list
> --all, like fsck) "forgets" about some worktree's refs and you will
> see what you see.
Good. I just wanted to be sure it was a known problem.
Thanks for the info.
Luc
^ permalink raw reply
* Re: [PATCH 1/2] difftool: add the builtin
From: Johannes Schindelin @ 2016-11-23 11:34 UTC (permalink / raw)
To: David Aguilar; +Cc: git, Junio C Hamano
In-Reply-To: <20161123080850.GA23742@gmail.com>
Hi David,
On Wed, 23 Nov 2016, David Aguilar wrote:
> On Tue, Nov 22, 2016 at 06:01:23PM +0100, Johannes Schindelin wrote:
>
> > +static const char * const builtin_difftool_usage[] = {
> > + N_("git add [<options>] [--] <pathspec>..."),
> > + NULL
> > +};
>
> The usage should probably say "difftool" (or "builtin-difftool").
Ah, my dirty secret was spilled. I copy-edited this. *pours ashes over his
head*
> > [...]
> > +static void changed_files(struct hashmap *result, const char *index_path,
> > + const char *workdir)
> > +{
> > +[...]
> > +}
> > +
> > +#include "dir.h"
>
> Can this mid-file #include go to the top of the file?
Yep, thanks.
In case you are interested: You probably guessed it, it was left for a
later clean-up. I worked a bit over the last weeks on getting Git to build
in Visual Studio, to be able to benefit from its quite nice features (I
was always a fan of Visual Studio, long before I started working at
Microsoft). I used the conversion of the difftool as an excuse to make use
of this myself: I did the entire conversion in Visual Studio, reverting to
the old, tedious command-line driven workflow to fix the bugs identified
by t7800-difftool.sh.
> > +static int run_dir_diff(const char *extcmd, int symlinks,
> > + int argc, const char **argv)
> > +{
> > + char tmpdir[PATH_MAX];
> > + struct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;
> > + struct strbuf rpath = STRBUF_INIT, buf = STRBUF_INIT;
> > + struct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT;
> > + struct strbuf wtdir = STRBUF_INIT;
> > + size_t ldir_len, rdir_len, wtdir_len;
> > + struct cache_entry *ce = xcalloc(1, sizeof(ce) + PATH_MAX + 1);
> > + const char *workdir, *tmp;
> > + int ret = 0, i;
> > + FILE *fp;
> > + struct hashmap working_tree_dups, submodules, symlinks2;
> > + struct hashmap_iter iter;
> > + struct pair_entry *entry;
> > + enum object_type type;
> > + unsigned long size;
> > + struct index_state wtindex;
> > + struct checkout lstate, rstate;
> > + int rc, flags = RUN_GIT_CMD, err = 0;
> > + struct child_process child = CHILD_PROCESS_INIT;
> > + const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
> > + struct hashmap wt_modified, tmp_modified;
> > + int indices_loaded = 0;
> > +
> > + setup_work_tree();
> > + workdir = get_git_work_tree();
> > +
> > + /* Setup temp directories */
> > + tmp = getenv("TMPDIR");
> > + sprintf(tmpdir, "%s/git-difftool.XXXXXX", tmp ? tmp : "/tmp");
>
> Maybe snprintf instead?
>
> getenv() won't return anything longer than PATH_MAX for most
> users, but users are weird.
True.
> > + if (!mkdtemp(tmpdir))
> > + return error("could not create temporary directory");
>
> Mention the tmpdir here?
Sure thing.
> > + strbuf_addf(&ldir, "%s/left/", tmpdir);
> > + strbuf_addf(&rdir, "%s/right/", tmpdir);
> > + strbuf_addstr(&wtdir, workdir);
> > + if (!wtdir.len || !is_dir_sep(wtdir.buf[wtdir.len - 1]))
> > + strbuf_addch(&wtdir, '/');
> > + mkdir(ldir.buf, 0777);
> > + mkdir(rdir.buf, 0777);
>
> Seeing the perl mkpath() default 0777 spelled out this way
> makes me wonder whether 0700 would be safer.
>
> The mkdtemp() above is already using 0700 so it's ok, but it
> might be worth making it consistent (later, perhaps).
Ah, of course! I stupidly imitated other `mkdir()` calls elsewhere, but
they refer to directories within the Git worktree...
> > + /*
> > + * In directory diff mode, 'git-difftool--helper' is called once
> > + * to compare the a / b directories.In file diff mode, 'git diff'
> > + * will invoke a separate instance of 'git-difftool--helper' for
> > + * each file that changed.
> > + */
>
> Missing space after "." in the comment above.
Yep. It was two spaces and I deleted one too many (we are so way past
actual print, where the two spaces may have made sense...).
> > + if (dir_diff)
> > + return run_dir_diff(extcmd, symlinks, argc, argv);
> > + return run_file_diff(prompt, argc, argv);
> > +}
> > diff --git a/git.c b/git.c
> > index efa1059..eaa0f67 100644
> > --- a/git.c
> > +++ b/git.c
> > @@ -424,6 +424,7 @@ static struct cmd_struct commands[] = {
> > { "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
> > { "diff-index", cmd_diff_index, RUN_SETUP },
> > { "diff-tree", cmd_diff_tree, RUN_SETUP },
> > + { "builtin-difftool", cmd_builtin_difftool, RUN_SETUP | NEED_WORK_TREE },
> > { "fast-export", cmd_fast_export, RUN_SETUP },
> > { "fetch", cmd_fetch, RUN_SETUP },
> > { "fetch-pack", cmd_fetch_pack, RUN_SETUP },
>
> This isn't alphabetical anymore, but it actually is if you
> consider that the final plan is to change "builtin-difftool" to
> "difftool".
Exactly, that was my thinking.
> If we want to minimize that future diff we could name
> cmd_builtin_difftool() as cmd_difftool() for consistency now so
> that the future commit only needs to tweak the string here.
Yes!
For the record, this is a left-over from an impatient attempt at avoiding
problems with `make` overwriting the Perl version of `git difftool` by the
builtin version; I had originally assumed that a list of builtins was
generated from parsing git.c or builtin.h, but it turns out that the
BUILTIN_OBJS are actually responsible, i.e. the file name.
Fixed.
Thank you for your review!
Dscho
^ permalink raw reply
* Re: [PATCH 1/3] rebase -i: highlight problems with core.commentchar
From: Johannes Schindelin @ 2016-11-23 11:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqwpfvqwbq.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 22 Nov 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Mon, 21 Nov 2016, Junio C Hamano wrote:
> >
> >> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> >> index 29e91d861c..c1f6411eb2 100755
> >> --- a/t/t0030-stripspace.sh
> >> +++ b/t/t0030-stripspace.sh
> >> @@ -432,6 +432,15 @@ test_expect_success '-c with changed comment char' '
> >> test_cmp expect actual
> >> '
> >>
> >> +test_expect_failure '-c with comment char defined in .git/config' '
> >> + test_config core.commentchar = &&
> >> + printf "= foo\n" >expect &&
> >> + printf "foo" | (
> >
> > Could I ask you to sneak in a \n here?
>
> The one before that _is_ about fixing incomplete line, but this and
> the one before this one are not, so I think we should fix them at
> the same time.
>
> But does it break anything to leave it as-is? If not, I'd prefer to
> leave this (and one before this one) for a later clean-up patch post
> release.
Fair enough.
As a matter of fact, we do not even need to change it later. If it ain't
broke, don't fix it.
Ciao,
Dscho
^ permalink raw reply
* Re: dangling commits in worktree
From: Duy Nguyen @ 2016-11-23 9:45 UTC (permalink / raw)
To: Van Oostenryck Luc; +Cc: Git Mailing List
In-Reply-To: <CAExDi1SYOuq7GJC69+5yDmzaw--vKMmmqv0Jsm80hU1L5phDUg@mail.gmail.com>
On Wed, Nov 23, 2016 at 7:52 AM, Van Oostenryck Luc
<luc.vanoostenryck@gmail.com> wrote:
> Hi,
>
> More or less by error I used the fsck command in a worktree and I had
> the surprised to see that it reported a lot of dangling commits while it was
> not supposed to have one.
> I quickly realized that it was the case only in the worktree, in the main dir
> things were OK. While experimenting a bit I also saw that git gc had not the
> same effect in a worktree than in the main tree (the pack was smaller, more
> files were left in objects/xx/ dirs), which is even more odd and a bit
> scary when thinking to the pruning.
>
> This seems like a bug to me and googling about it didn't returned anything.
It's a known issue that gc (and maybe some others that do rev-list
--all, like fsck) "forgets" about some worktree's refs and you will
see what you see. Work on it was postponed because the "refs"
subsystem was being refactored. I think I'm resuming it soon.
--
Duy
^ permalink raw reply
* Re: [PATCH] merge-recursive.c: use QSORT macro
From: Duy Nguyen @ 2016-11-23 9:43 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, René Scharfe
In-Reply-To: <20161122174946.jy5at4g7rifu3und@sigill.intra.peff.net>
On Wed, Nov 23, 2016 at 12:49 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Nov 22, 2016 at 07:30:19PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> This is the follow up of rs/qsort series, merged in b8688ad (Merge
>> branch 'rs/qsort' - 2016-10-10), where coccinelle was used to do
>> automatic transformation.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>> coccinelle missed this place, understandably, because it can't know
>> that
>>
>> sizeof(*entries->items)
>>
>> is the same as
>>
>> sizeof(*df_name_compare.items)
>>
>> without some semantic analysis.
>
> That made me wonder why "entries" is used at all. Does it point to the
> same struct? But no, df_name_compare is a string list we create with the
> same list of strings.
>
> Which is why...
>
>> - qsort(df_sorted_entries.items, entries->nr, sizeof(*entries->items),
>> + QSORT(df_sorted_entries.items, entries->nr,
>> string_list_df_name_compare);
>
> ...it's OK to use entries->nr here, and not df_sorted_entries.nr. It
> still seems a bit odd, though.
Argh.. I completely overlooked that entries->nr !
> Maybe it's worth making this:
>
> QSORT(df_sorted_entries.items, df_sorted_entries.nr,
> string_list_df_name_compare);
>
> while we're at it. Another possibility is:
>
> df_sorted_entries.cmp = string_list_df_name_compare;
> string_list_sort(&df_sorted_entries);
>
> It's not any shorter, but maybe it's conceptually simpler.
Agreed. Shall I re-roll with this?
--
Duy
^ permalink raw reply
* Re: [PATCH 31/35] pathspec: allow querying for attributes
From: Duy Nguyen @ 2016-11-23 9:38 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, Brandon Williams, Git Mailing List
In-Reply-To: <CAGZ79kYm4LfXK=1j-ayLawt+BojnkyM4h2RLQ=kfpPgMQbdBag@mail.gmail.com>
On Wed, Nov 23, 2016 at 12:26 AM, Stefan Beller <sbeller@google.com> wrote:
> On Tue, Nov 22, 2016 at 2:41 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Fri, Nov 11, 2016 at 3:34 AM, Stefan Beller <sbeller@google.com> wrote:
>>> @@ -139,7 +140,8 @@ static size_t common_prefix_len(const struct pathspec *pathspec)
>>> PATHSPEC_LITERAL |
>>> PATHSPEC_GLOB |
>>> PATHSPEC_ICASE |
>>> - PATHSPEC_EXCLUDE);
>>> + PATHSPEC_EXCLUDE |
>>> + PATHSPEC_ATTR);
>>
>> Hmm.. common_prefix_len() has always been a bit relaxing and can cover
>> more than needed. It's for early pruning. Exact pathspec matching
>> _will_ be done later anyway.
>>
>> Is that obvious?
>
> Yes it is.
> Not sure what your concern is, though.
None really. I was just thinking out loud and trying not to make
assumptions, because I know this code quite well and I don't know how
people see this code anymore :D So all is good then.
--
Duy
^ permalink raw reply
* Re: [PATCH 1/2] difftool: add the builtin
From: David Aguilar @ 2016-11-23 8:08 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <7aec571e1b0773ca80ea25453d2650af6a18e095.1479834051.git.johannes.schindelin@gmx.de>
On Tue, Nov 22, 2016 at 06:01:23PM +0100, Johannes Schindelin wrote:
> This adds a builtin difftool that represents a conversion of the current
> Perl script version of the difftool.
>
> The motivation is that Perl scripts are not at all native on Windows,
> and that `git difftool` therefore is pretty slow on that platform, when
> there is no good reason for it to be slow.
>
> In addition, Perl does not really have access to Git's internals. That
> means that any script will always have to jump through unnecessary
> hoops.
Nice!
> The current version of the builtin difftool does not, however, make full
> use of the internals but instead chooses to spawn a couple of Git
> processes, still, to make for an easier conversion. There remains a lot
> of room for improvement, left for a later date.
>
> Note: the original difftool is still called by `git difftool`. To get the
> new, experimental version, call `git builtin-difftool`. The reason: this
> new, experimental, builtin difftool will be shipped as part of Git for
> Windows v2.11.0, to allow for easier large-scale testing.
I like this plan. I was going to ask for an environment
variable (to preset in git-cola) but since Git for Windows is
handling it then everyone benefits.
> diff --git a/builtin/builtin-difftool.c b/builtin/builtin-difftool.c
> new file mode 100644
> index 0000000..9feefcd
> --- /dev/null
> +++ b/builtin/builtin-difftool.c
> @@ -0,0 +1,680 @@
> +/*
> + * "git difftool" builtin command
> + *
> + * This is a wrapper around the GIT_EXTERNAL_DIFF-compatible
> + * git-difftool--helper script.
> + *
> + * This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git.
> + * The GIT_DIFF* variables are exported for use by git-difftool--helper.
> + *
> + * Any arguments that are unknown to this script are forwarded to 'git diff'.
> + *
> + * Copyright (C) 2016 Johannes Schindelin
> + */
> +#include "cache.h"
> +#include "builtin.h"
> +#include "parse-options.h"
> +#include "run-command.h"
> +#include "argv-array.h"
> +#include "strbuf.h"
> +#include "lockfile.h"
> +
> +static char *diff_gui_tool;
> +static int trust_exit_code;
> +
> +static const char * const builtin_difftool_usage[] = {
> + N_("git add [<options>] [--] <pathspec>..."),
> + NULL
> +};
The usage should probably say "difftool" (or "builtin-difftool").
> [...]
> +static void changed_files(struct hashmap *result, const char *index_path,
> + const char *workdir)
> +{
> +[...]
> +}
> +
> +#include "dir.h"
Can this mid-file #include go to the top of the file?
> +static int run_dir_diff(const char *extcmd, int symlinks,
> + int argc, const char **argv)
> +{
> + char tmpdir[PATH_MAX];
> + struct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;
> + struct strbuf rpath = STRBUF_INIT, buf = STRBUF_INIT;
> + struct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT;
> + struct strbuf wtdir = STRBUF_INIT;
> + size_t ldir_len, rdir_len, wtdir_len;
> + struct cache_entry *ce = xcalloc(1, sizeof(ce) + PATH_MAX + 1);
> + const char *workdir, *tmp;
> + int ret = 0, i;
> + FILE *fp;
> + struct hashmap working_tree_dups, submodules, symlinks2;
> + struct hashmap_iter iter;
> + struct pair_entry *entry;
> + enum object_type type;
> + unsigned long size;
> + struct index_state wtindex;
> + struct checkout lstate, rstate;
> + int rc, flags = RUN_GIT_CMD, err = 0;
> + struct child_process child = CHILD_PROCESS_INIT;
> + const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
> + struct hashmap wt_modified, tmp_modified;
> + int indices_loaded = 0;
> +
> + setup_work_tree();
> + workdir = get_git_work_tree();
> +
> + /* Setup temp directories */
> + tmp = getenv("TMPDIR");
> + sprintf(tmpdir, "%s/git-difftool.XXXXXX", tmp ? tmp : "/tmp");
Maybe snprintf instead?
getenv() won't return anything longer than PATH_MAX for most
users, but users are weird.
> + if (!mkdtemp(tmpdir))
> + return error("could not create temporary directory");
Mention the tmpdir here?
> + strbuf_addf(&ldir, "%s/left/", tmpdir);
> + strbuf_addf(&rdir, "%s/right/", tmpdir);
> + strbuf_addstr(&wtdir, workdir);
> + if (!wtdir.len || !is_dir_sep(wtdir.buf[wtdir.len - 1]))
> + strbuf_addch(&wtdir, '/');
> + mkdir(ldir.buf, 0777);
> + mkdir(rdir.buf, 0777);
Seeing the perl mkpath() default 0777 spelled out this way
makes me wonder whether 0700 would be safer.
The mkdtemp() above is already using 0700 so it's ok, but it
might be worth making it consistent (later, perhaps).
> [...]
> +int cmd_builtin_difftool(int argc, const char ** argv, const char * prefix)
> +{
> + int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
> + tool_help = 0;
> + static char *difftool_cmd = NULL, *extcmd = NULL;
> +
> + struct option builtin_difftool_options[] = {
> + OPT_BOOL('g', "gui", &use_gui_tool,
> + N_("use `diff.guitool` instead of `diff.tool`")),
> + OPT_BOOL('d', "dir-diff", &dir_diff,
> + N_("perform a full-directory diff")),
> + { OPTION_SET_INT, 'y', "no-prompt", &prompt, NULL,
> + N_("do not prompt before launching a diff tool"),
> + PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 0},
> + { OPTION_SET_INT, 0, "prompt", &prompt, NULL, NULL,
> + PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN,
> + NULL, 1 },
> + OPT_BOOL(0, "symlinks", &symlinks,
> + N_("use symlinks in dir-diff mode")),
> + OPT_STRING('t', "tool", &difftool_cmd, N_("<tool>"),
> + N_("use the specified diff tool")),
> + OPT_BOOL(0, "tool-help", &tool_help,
> + N_("print a list of diff tools that may be used with "
> + "`--tool`")),
> + OPT_BOOL(0, "trust-exit-code", &trust_exit_code,
> + N_("make 'git-difftool' exit when an invoked diff "
> + "tool returns a non - zero exit code")),
> + OPT_STRING('x', "extcmd", &extcmd, N_("<command>"),
> + N_("specify a custom command for viewing diffs")),
> + OPT_END()
> + };
> +
> + symlinks = has_symlinks;
> +
> + git_config(difftool_config, NULL);
> +
> + argc = parse_options(argc, argv, prefix, builtin_difftool_options,
> + builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN |
> + PARSE_OPT_KEEP_DASHDASH);
> +
> + if (tool_help)
> + return print_tool_help();
> +
> + if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
> + setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
> + else if (difftool_cmd) {
> + if (*difftool_cmd)
> + setenv("GIT_DIFF_TOOL", difftool_cmd, 1);
> + else
> + die(_("no <tool> given for --tool=<tool>"));
> + }
> +
> + if (extcmd) {
> + if (*extcmd)
> + setenv("GIT_DIFFTOOL_EXTCMD", extcmd, 1);
> + else
> + die(_("no <cmd> given for --extcmd=<cmd>"));
> + }
> +
> + setenv("GIT_DIFFTOOL_TRUST_EXIT_CODE",
> + trust_exit_code ? "true" : "false", 1);
> +
> + /*
> + * In directory diff mode, 'git-difftool--helper' is called once
> + * to compare the a / b directories.In file diff mode, 'git diff'
> + * will invoke a separate instance of 'git-difftool--helper' for
> + * each file that changed.
> + */
Missing space after "." in the comment above.
> + if (dir_diff)
> + return run_dir_diff(extcmd, symlinks, argc, argv);
> + return run_file_diff(prompt, argc, argv);
> +}
> diff --git a/git.c b/git.c
> index efa1059..eaa0f67 100644
> --- a/git.c
> +++ b/git.c
> @@ -424,6 +424,7 @@ static struct cmd_struct commands[] = {
> { "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
> { "diff-index", cmd_diff_index, RUN_SETUP },
> { "diff-tree", cmd_diff_tree, RUN_SETUP },
> + { "builtin-difftool", cmd_builtin_difftool, RUN_SETUP | NEED_WORK_TREE },
> { "fast-export", cmd_fast_export, RUN_SETUP },
> { "fetch", cmd_fetch, RUN_SETUP },
> { "fetch-pack", cmd_fetch_pack, RUN_SETUP },
This isn't alphabetical anymore, but it actually is if you
consider that the final plan is to change "builtin-difftool" to
"difftool".
If we want to minimize that future diff we could name
cmd_builtin_difftool() as cmd_difftool() for consistency now so
that the future commit only needs to tweak the string here.
--
David
^ permalink raw reply
* dangling commits in worktree
From: Van Oostenryck Luc @ 2016-11-23 0:52 UTC (permalink / raw)
To: git
Hi,
More or less by error I used the fsck command in a worktree and I had
the surprised to see that it reported a lot of dangling commits while it was
not supposed to have one.
I quickly realized that it was the case only in the worktree, in the main dir
things were OK. While experimenting a bit I also saw that git gc had not the
same effect in a worktree than in the main tree (the pack was smaller, more
files were left in objects/xx/ dirs), which is even more odd and a bit
scary when thinking to the pruning.
This seems like a bug to me and googling about it didn't returned anything.
Luc Van Oostenryck
^ permalink raw reply
* Re: [PATCH 2/3] stripspace: respect repository config
From: Jeff King @ 2016-11-23 0:12 UTC (permalink / raw)
To: Junio C Hamano
Cc: René Scharfe, Johannes Schindelin, Duy Nguyen,
Git Mailing List, Ralf Thielow, Taufiq Hoven
In-Reply-To: <xmqq7f7vp4ck.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 22, 2016 at 01:55:07PM -0800, Junio C Hamano wrote:
> > And this test makes sense. Even without "sub", it would show the
> > regression, but it's a good idea to test the sub-directory case to cover
> > the path-munging.
>
> Yup. Obviously during my initial attempt I was scratching my head
> wondering where these two files went--they were later found inside
> t/ directory which was really bad ;-)
Heh. Yeah, it's nice when the failure mode doesn't escape the trash
directory, but it's probably not worth worrying about too much.
> > In the "archive --remote" test I added, we may want to do the same to
> > show that "--output" points at the correct path.
>
> Perhaps something like this. By going down one level, we make sure
> that it is not sufficient to accidentally read from .git/config to
> find out what 'foo' is, and also ../b5-nick.tar that is relative to
> the prefix (aka 'a/') ends up at the top-level.
Yeah, your modification looks good.
-Peff
^ permalink raw reply
* Re: [PATCH v5 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Brandon Williams @ 2016-11-22 23:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, jonathantanmy
In-Reply-To: <xmqqk2bvnl1e.fsf@gitster.mtv.corp.google.com>
On 11/22, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> >> > So this change may have an impact on "git ls-tree -r" with pathspec;
> >> > I offhand do not know if that impact is undesirable or not. A test
> >> > or two may be in order to illustrate what happens? With a submodule
> >> > at "sub/module", running "git ls-tree -r HEAD -- sub/module/*" or
> >> > something like that, perhaps?
> >>
> >> Maybe unrelated, but it looks like wildcard characters are overridden in
> >> ls-tree.c per '170260ae'. As such wildmatching just doesn't work with
> >> ls-tree. so `git ls-tree -r HEAD -- "*"` results in no hits.
> >
> > Wrong commit. Its this one (f0096c06bcdeb7aa6ae8a749ddc9d6d4a2c381d1)
> > that disabled wildmatching since it is 'plumbing'
>
> OK. Things that share tree-walk other than "ls-tree -r" are still
> affected, no?
Yeah potentially, though I'm having a difficult time finding a case that
would actually be affected.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v5 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Junio C Hamano @ 2016-11-22 23:37 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, jonathantanmy
In-Reply-To: <20161122232815.GD65825@google.com>
Brandon Williams <bmwill@google.com> writes:
>> > So this change may have an impact on "git ls-tree -r" with pathspec;
>> > I offhand do not know if that impact is undesirable or not. A test
>> > or two may be in order to illustrate what happens? With a submodule
>> > at "sub/module", running "git ls-tree -r HEAD -- sub/module/*" or
>> > something like that, perhaps?
>>
>> Maybe unrelated, but it looks like wildcard characters are overridden in
>> ls-tree.c per '170260ae'. As such wildmatching just doesn't work with
>> ls-tree. so `git ls-tree -r HEAD -- "*"` results in no hits.
>
> Wrong commit. Its this one (f0096c06bcdeb7aa6ae8a749ddc9d6d4a2c381d1)
> that disabled wildmatching since it is 'plumbing'
OK. Things that share tree-walk other than "ls-tree -r" are still
affected, no?
^ permalink raw reply
* Announce: delaying 2.11 final by one week
From: Junio C Hamano @ 2016-11-22 23:35 UTC (permalink / raw)
To: git
We'd like to cook and merge a few hot-fix topics to 'master' before
the upcoming release, in order to fix minor regressions in
"stripspace" (which affects "rebase -i"), "archive" and "mailinfo"
that were introduced during this cycle. So I'll be cutting -rc3
soon this week, and hopefully tagging the final by the end of this
month.
Thanks.
^ permalink raw reply
* Re: [PATCH v5 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Brandon Williams @ 2016-11-22 23:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, jonathantanmy
In-Reply-To: <20161122232105.GC65825@google.com>
On 11/22, Brandon Williams wrote:
> On 11/22, Junio C Hamano wrote:
> > Brandon Williams <bmwill@google.com> writes:
> >
> > > diff --git a/tree-walk.c b/tree-walk.c
> > > index 828f435..ff77605 100644
> > > --- a/tree-walk.c
> > > +++ b/tree-walk.c
> > > @@ -1004,6 +1004,19 @@ static enum interesting do_match(const struct name_entry *entry,
> > > */
> > > if (ps->recursive && S_ISDIR(entry->mode))
> > > return entry_interesting;
> > > +
> > > + /*
> > > + * When matching against submodules with
> > > + * wildcard characters, ensure that the entry
> > > + * at least matches up to the first wild
> > > + * character. More accurate matching can then
> > > + * be performed in the submodule itself.
> > > + */
> > > + if (ps->recursive && S_ISGITLINK(entry->mode) &&
> > > + !ps_strncmp(item, match + baselen,
> > > + entry->path,
> > > + item->nowildcard_len - baselen))
> > > + return entry_interesting;
> > > }
> >
> > This one (and the other hunk) feels more correct than the previous
> > round. One thing to keep in mind however is that ps->recursive is
> > about "do we show a tree as a tree aka 040000, or do we descend into
> > it to show its contents?", not about "do we recurse into submodules?",
> > AFAICT.
> >
> > So this change may have an impact on "git ls-tree -r" with pathspec;
> > I offhand do not know if that impact is undesirable or not. A test
> > or two may be in order to illustrate what happens? With a submodule
> > at "sub/module", running "git ls-tree -r HEAD -- sub/module/*" or
> > something like that, perhaps?
>
> Maybe unrelated, but it looks like wildcard characters are overridden in
> ls-tree.c per '170260ae'. As such wildmatching just doesn't work with
> ls-tree. so `git ls-tree -r HEAD -- "*"` results in no hits.
Wrong commit. Its this one (f0096c06bcdeb7aa6ae8a749ddc9d6d4a2c381d1)
that disabled wildmatching since it is 'plumbing'
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v5 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Brandon Williams @ 2016-11-22 23:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, jonathantanmy
In-Reply-To: <xmqqshqjnmtc.fsf@gitster.mtv.corp.google.com>
On 11/22, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > diff --git a/tree-walk.c b/tree-walk.c
> > index 828f435..ff77605 100644
> > --- a/tree-walk.c
> > +++ b/tree-walk.c
> > @@ -1004,6 +1004,19 @@ static enum interesting do_match(const struct name_entry *entry,
> > */
> > if (ps->recursive && S_ISDIR(entry->mode))
> > return entry_interesting;
> > +
> > + /*
> > + * When matching against submodules with
> > + * wildcard characters, ensure that the entry
> > + * at least matches up to the first wild
> > + * character. More accurate matching can then
> > + * be performed in the submodule itself.
> > + */
> > + if (ps->recursive && S_ISGITLINK(entry->mode) &&
> > + !ps_strncmp(item, match + baselen,
> > + entry->path,
> > + item->nowildcard_len - baselen))
> > + return entry_interesting;
> > }
>
> This one (and the other hunk) feels more correct than the previous
> round. One thing to keep in mind however is that ps->recursive is
> about "do we show a tree as a tree aka 040000, or do we descend into
> it to show its contents?", not about "do we recurse into submodules?",
> AFAICT.
>
> So this change may have an impact on "git ls-tree -r" with pathspec;
> I offhand do not know if that impact is undesirable or not. A test
> or two may be in order to illustrate what happens? With a submodule
> at "sub/module", running "git ls-tree -r HEAD -- sub/module/*" or
> something like that, perhaps?
Maybe unrelated, but it looks like wildcard characters are overridden in
ls-tree.c per '170260ae'. As such wildmatching just doesn't work with
ls-tree. so `git ls-tree -r HEAD -- "*"` results in no hits.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v5 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Junio C Hamano @ 2016-11-22 22:59 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, jonathantanmy
In-Reply-To: <1479840397-68264-6-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> diff --git a/tree-walk.c b/tree-walk.c
> index 828f435..ff77605 100644
> --- a/tree-walk.c
> +++ b/tree-walk.c
> @@ -1004,6 +1004,19 @@ static enum interesting do_match(const struct name_entry *entry,
> */
> if (ps->recursive && S_ISDIR(entry->mode))
> return entry_interesting;
> +
> + /*
> + * When matching against submodules with
> + * wildcard characters, ensure that the entry
> + * at least matches up to the first wild
> + * character. More accurate matching can then
> + * be performed in the submodule itself.
> + */
> + if (ps->recursive && S_ISGITLINK(entry->mode) &&
> + !ps_strncmp(item, match + baselen,
> + entry->path,
> + item->nowildcard_len - baselen))
> + return entry_interesting;
> }
This one (and the other hunk) feels more correct than the previous
round. One thing to keep in mind however is that ps->recursive is
about "do we show a tree as a tree aka 040000, or do we descend into
it to show its contents?", not about "do we recurse into submodules?",
AFAICT.
So this change may have an impact on "git ls-tree -r" with pathspec;
I offhand do not know if that impact is undesirable or not. A test
or two may be in order to illustrate what happens? With a submodule
at "sub/module", running "git ls-tree -r HEAD -- sub/module/*" or
something like that, perhaps?
^ permalink raw reply
* Re: [PATCHv4 0/3] submodule-config: clarify/cleanup docs and header
From: Brandon Williams @ 2016-11-22 22:31 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git, jacob.keller
In-Reply-To: <20161122201438.16069-1-sbeller@google.com>
Series looks good to me. At least the issues I raised are fixed.
--
Brandon Williams
^ permalink raw reply
* Re: [GIT PULL] l10n updates for 2.11.0 round 2
From: Junio C Hamano @ 2016-11-22 22:24 UTC (permalink / raw)
To: Jiang Xin
Cc: Alexander Shopov, Alex Henrie, Ralf Thielow, Jean-Noël Avila,
Marco Paolone, Changwoo Ryu, Vasco Almeida, Dimitriy Ryazantcev,
Peter Krefting, Trần Ngọc Quân, Git List
In-Reply-To: <CANYiYbFSpUmtv1eO_GE_yRU4CsXzd29qV2LTbXa67d=Uxbw2=Q@mail.gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> Git 2.11.0-rc2 introduced a very small l10n update. Because the
> update windows will be
> closed tomorrow, I made a batch updates. See commit:
>
> * https://github.com/git-l10n/git-po/commit/275588f93
>
> I have a reduced diff for this commit using a custom diff driver, see:
>
> * https://gist.github.com/jiangxin/6384b1e865249228e00385fab84ef3f3
>
>
> 2016-11-22 22:59 GMT+08:00 Jiang Xin <worldhello.net@gmail.com>:
>> Hi Junio,
>>
>> The following changes since commit 1310affe024fba407bff55dbe65cd6d670c8a32d:
>>
>> Git 2.11-rc2 (2016-11-17 13:47:36 -0800)
>>
>> are available in the git repository at:
>>
>> git://github.com/git-l10n/git-po tags/l10n-2.11.0-rnd2
Thanks, pulled.
FYI, we seem to need a bit more time on the code front so we'll have
an unscheduled 2.11-rc3 tomorrow. The final one will be near the
end of the month, tenatively scheduled on Nov 29th.
^ permalink raw reply
* Re: [PATCH 2/3] stripspace: respect repository config
From: Junio C Hamano @ 2016-11-22 21:55 UTC (permalink / raw)
To: Jeff King
Cc: René Scharfe, Johannes Schindelin, Duy Nguyen,
Git Mailing List, Ralf Thielow, Taufiq Hoven
In-Reply-To: <20161122214305.yrn4uqh4dzzafkd2@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> +test_expect_success 'mailinfo with mailinfo.scissors config' '
>> + test_config mailinfo.scissors true &&
>> + (
>> + mkdir sub &&
>> + cd sub &&
>> + git mailinfo ../msg0014.sc ../patch0014.sc <../0014 >../info0014.sc
>> + ) &&
>> + test_cmp "$DATA/msg0014--scissors" msg0014.sc &&
>> + test_cmp "$DATA/patch0014--scissors" patch0014.sc &&
>> + test_cmp "$DATA/info0014--scissors" info0014.sc
>> +'
>
> And this test makes sense. Even without "sub", it would show the
> regression, but it's a good idea to test the sub-directory case to cover
> the path-munging.
Yup. Obviously during my initial attempt I was scratching my head
wondering where these two files went--they were later found inside
t/ directory which was really bad ;-)
> In the "archive --remote" test I added, we may want to do the same to
> show that "--output" points at the correct path.
Perhaps something like this. By going down one level, we make sure
that it is not sufficient to accidentally read from .git/config to
find out what 'foo' is, and also ../b5-nick.tar that is relative to
the prefix (aka 'a/') ends up at the top-level.
t/t5000-tar-tree.sh | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 09df7f0458..830bf2a2f6 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -195,7 +195,10 @@ test_expect_success 'git archive --remote' \
test_expect_success 'git archive --remote with configured remote' '
git config remote.foo.url . &&
- git archive --remote=foo HEAD >b5-nick.tar &&
+ (
+ cd a &&
+ git archive --remote=foo --output=../b5-nick.tar HEAD
+ ) &&
test_cmp_bin b.tar b5-nick.tar
'
^ permalink raw reply related
* Re: [PATCH 2/3] stripspace: respect repository config
From: Jeff King @ 2016-11-22 21:43 UTC (permalink / raw)
To: Junio C Hamano
Cc: René Scharfe, Johannes Schindelin, Duy Nguyen,
Git Mailing List, Ralf Thielow, Taufiq Hoven
In-Reply-To: <xmqqeg23p5v3.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 22, 2016 at 01:22:24PM -0800, Junio C Hamano wrote:
> OK. The "mailinfo" part turns out a bit more than RUN_SETUP_GENTLY
> as it takes paths from the command line that needs to be adjusted
> for the prefix.
I wondered at first if our lack of parse-options here was holding us
back from using OPT_FILENAME. Though even if that was the case, it is
probably better to do the minimal fix in this instance, rather than
converting the option parsing.
However, the paths which need munged are not even options, they are
arguments. So we wouldn't be able to rely on OPT_FILENAME anyway. I'm
surprised we haven't had to deal with this elsewhere, but I guess most
commands don't take arbitrary filenames (things like `add` take
pathspecs, and then the pathspec machinery takes care of the details).
The only other case I could think of is git-apply, and it does use
prefix_filename() correctly.
> +static char *prefix_copy(const char *prefix, const char *filename)
> +{
> + if (!prefix || is_absolute_path(filename))
> + return xstrdup(filename);
> + return xstrdup(prefix_filename(prefix, strlen(prefix), filename));
> +}
So this is more or less a copy of parse-option's fix_filename(), which
makes sense.
I noticed that git-apply does not check is_absolute_path(), but that's
OK, as prefix_filename() does. So I think it would be correct to drop it
here, but it doesn't matter much either way.
> int cmd_mailinfo(int argc, const char **argv, const char *prefix)
> {
> const char *def_charset;
> struct mailinfo mi;
> int status;
> + char *msgfile, *patchfile;
>
> - /* NEEDSWORK: might want to do the optional .git/ directory
> - * discovery
> - */
> setup_mailinfo(&mi);
This part looks straightforward and correct. Dropping the NEEDSWORK is a
nice bonus.
> +test_expect_success 'mailinfo with mailinfo.scissors config' '
> + test_config mailinfo.scissors true &&
> + (
> + mkdir sub &&
> + cd sub &&
> + git mailinfo ../msg0014.sc ../patch0014.sc <../0014 >../info0014.sc
> + ) &&
> + test_cmp "$DATA/msg0014--scissors" msg0014.sc &&
> + test_cmp "$DATA/patch0014--scissors" patch0014.sc &&
> + test_cmp "$DATA/info0014--scissors" info0014.sc
> +'
And this test makes sense. Even without "sub", it would show the
regression, but it's a good idea to test the sub-directory case to cover
the path-munging.
In the "archive --remote" test I added, we may want to do the same to
show that "--output" points at the correct path.
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox