On Fri, Nov 19 2021, Teng Long wrote:
> Reviewed-by: Jeff King <peff@xxxxxxxx>
> Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> Reviewed-by: Đoàn Trần Công Danh <congdanhqx@xxxxxxxxx>
Please don't add the Reviewed-by headers yourself, either Junio
accumulates them, or if someone explicitly mentions that you can add it
with their name it's OK.
It doesn't just mean this person reviewed this series in some ML thread,
but "this person is 100% OK with this in its current form".
> List only filenames (instead of the "long" output), one per line.
> -
> + Cannot be used with `--oid-only` together.
Better: "Cannot be combined with OPT."
> +--oid-only::
> + List only OIDs of the objects, one per line. Cannot be used with
> + `--name-only` or `--name-status` together.
Better: "Cannot be combined with OPT or OPT2."
> +enum {
> + MODE_UNSPECIFIED = 0,
> + MODE_NAME_ONLY,
> + MODE_OID_ONLY
> +};
> +
> +static int cmdmode = MODE_UNSPECIFIED;
Better:
static enum {
MODE_NAME_ONLY = 1,
...
} cmdmode = MODE_NAME_ONLY;
I.e. no need for the MODE_UNSPECIFIED just to skip past "0".
> @@ -135,10 +147,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
> N_("terminate entries with NUL byte"), 0),
> OPT_BIT('l', "long", &ls_options, N_("include object size"),
> LS_SHOW_SIZE),
> - OPT_BIT(0, "name-only", &ls_options, N_("list only filenames"),
> - LS_NAME_ONLY),
> - OPT_BIT(0, "name-status", &ls_options, N_("list only filenames"),
> - LS_NAME_ONLY),
> + OPT_CMDMODE('n', "name-only", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY),
> + OPT_CMDMODE('s', "name-status", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY),
> + OPT_CMDMODE('o', "oid-only", &cmdmode, N_("list only oids"), MODE_OID_ONLY),
Better to preserve the wrapping here, to stay within 79 columns.
> +test_expect_success 'setup' '
> + echo 111 >1.txt &&
> + echo 222 >2.txt &&
Just use:
test_commit A &&
test_commit B
etc?
> + mkdir -p path0/a/b/c &&
> + echo 333 >path0/a/b/c/3.txt &&
> + find *.txt path* \( -type f -o -type l \) -print |
> + xargs git update-index --add &&
> + tree=$(git write-tree) &&
> + echo $tree
Stray echo? Unclear why this test setup is so complex, shouldn't this just be (continued from above):
mkdir -p C &&
test_commit C/D.txt
To test nested dirs?
> +'
> +
> +test_expect_success 'usage: --oid-only' '
> + git ls-tree --oid-only $tree >current &&
> + git ls-tree $tree | awk "{print \$3}" >expected &&
just cut -f1 instead of awk? Also don't put "git" on the LHS of a pipe,
it might hide segfaults. Also applies to the below.
> + test_cmp current expected
> +'
> +
> +test_expect_success 'usage: --oid-only with -r' '
> + git ls-tree --oid-only -r $tree >current &&
> + git ls-tree -r $tree | awk "{print \$3}" >expected &&
> + test_cmp current expected
> +'
> +
> +test_expect_success 'usage: --oid-only with --abbrev' '
> + git ls-tree --oid-only --abbrev=6 $tree >current &&
> + git ls-tree --abbrev=6 $tree | awk "{print \$3}" > expected &&
> + test_cmp current expected
> +'
> +
> +test_expect_failure 'usage: incompatible options: --name-only with --oid-only' '
> + test_incompatible_usage git ls-tree --oid-only --name-only
> +'
Hrm, did you copy this use of test_incompatible_usage from
t1006-cat-file.sh without providing the function?
More data for:
https://lore.kernel.org/git/87tuhmk19c.fsf@xxxxxxxxxxxxxxxxxxx/ :)
Better to use:
test_expect_code 128 ... # (or was it 129?)
