Expand 'show' command to display patches by craigds · Pull Request #48 · koordinates/kart

craigds · 2020-04-15T08:44:29Z

sno show already exists, but takes no options and actually just displays a text log entry for the given commit. This PR expands it to work much more like git show.

Shows a commit (default HEAD) as a patch, in either text
(a la git show) or JSON (with --json).

HTML and 'quiet' output could be added in future.

To do

add tests

Output

The patch is a diff plus some meta-information. The diff
is just the same as the sno diff output for the relevant
commit.

In text mode, the meta-information is the same as what
git show outputs:

commit ad9797ea83000864c2ab98c421d2d5256a12db8f
Author: Craig de Stigter <craig@destigter.nz>
Date:   Wed Apr 15 13:19:16 2020 +1200

    commit

<diff output follows>

In JSON mode, the metadata is added to the top level JSON object
so that the patch is also a diff. It looks like:

{
  "sno.diff/v1": {...},
  "sno.patch/v1": {
    "authorName": "Craig de Stigter",
    "authorEmail": "craig@destigter.nz",
    "authorTime": "2020-04-15T01:19:16Z",
    "authorTimeOffset": "+12:00",
    "message": "commit"
  }
}

rcoup

Looking promising!

Feels like we'll need the console pager to work reliably cross-platform though if we're including massive diffs.

Possible to split the refactoring from the new additions into separate commits?

rcoup · 2020-04-15T10:36:23Z

+    elif (not output_path) or output_path == "-":
+        return sys.stdout
+    else:
+        return output_path.open("w")


Should we use a mode here?

output_path can be a directory as well (eg: for geojson files)

Should we use a mode here?

I assume you mean "a" for append-mode. I think probably not? For most formats (json, geojson, html) it doesn't make sense to append. I don't know why you'd want to for text either. if you need to you can just use - and then use >> outfile to do it in your shell

output_path can be a directory as well (eg: for geojson files)

this function can't handle that, but diff_output_geojson deals with it specially

I assume you mean "a" for append-mode.

I meant text-mode with an encoding, since we're writing text?

ah right.

I don't think this is an issue nowadays? Everything defaults to UTF-8 in recent python versions, even if you have other locale settings:

LANG="en_NZ.latin-1" LC_COLLATE="en_NZ.latin-1" LC_CTYPE="en_NZ.latin-1" LC_MESSAGES="en_NZ.latin-1" LC_MONETARY="en_NZ.latin-1" LC_NUMERIC="en_NZ.latin-1" LC_TIME="en_NZ.latin-1" LC_ALL= ipython3 ... >>> import sys, locale, os >>> locale.getpreferredencoding(True), sys.getdefaultencoding() ('UTF-8', 'utf-8')

craigds · 2020-04-15T21:12:17Z

Possible to split the refactoring from the new additions into separate commits?

Sure I can split it up a bit.

Feels like we'll need the console pager to work reliably cross-platform though if we're including massive diffs.

I'm not really sure what this refers to; is there some context somewhere? The existing sno diff output isn't much smaller than the output of sno show, so it sounds like if the console pager isn't working well we should fix that regardless. That can go in a separate change though?

rcoup · 2020-04-16T09:23:12Z

I'm not really sure what this refers to; is there some context somewhere?

I'm not sure the console pager works at all unless we're exec'ing git.

, so it sounds like if the console pager isn't working well we should fix that regardless. That can go in a separate change though?

Can you ticket?

rcoup · 2020-04-16T09:28:42Z

Rethinking:

This PR expands it to work much more like git show.
The existing sno diff output isn't much smaller than the output of sno show

I think we need to revisit what the goal is here — why are we making this change? "matching git" isn't automatically desirable, we want to avoid a pile of historic git mis-steps and make it usable for our audience. Datasets aren't code.

Personally i don't see the point of sno show returning a diff, seems like as an (effective) alias of log -1 it's more useful than as an alias of log -1 -p. For me that shows the context of "what's the last change made? where did I get up to?", and I feel 100s of lines of IDs doesn't seem particularly helpful in that regard? Though I'm but one user, and I have my own ways of working that are different too. Thoughts @hamishcampbell?

craigds · 2020-04-16T21:00:53Z

i'm a bit confused about it... if you don't use show to show what's in your commit, how would you view what's in your commit?

I guess sno diff {sha1}^..{sha1} would work, but it's annoying to type, and that won't show the log message, so to get all the context for what's in the commit you need to run two commands?

Why not just make show do what git does, and show the whole commit?
or are you saying text diffs aren't useful at all for data?

craigds · 2020-04-19T21:49:41Z

talked with @rcoup out-of-band, he said:

Sure, ok.

I guess I pretty rarely use show {sha}, I’m more likely to do log -p. show (no args) I kinda use to see what the last change is and whether it’s mine 😀

To which I commented

I use them both in tandem - I have a git log open in one screen, and when i find a commit i think i might care about, i use git show {sha} in a separate window

IMHO since there's no objective or consensus reason to differ from general git behaviour here, we should stick closely to what git does.

As @rcoup pointed out, it does seem important to implement an output pager so that the commit message and author info is visible on screen. That's tracked in #50 and can be implemented as a separate effort

Refactor output handling so it can be re-used by the show command.

`sno show` already exists, but takes no options and actually just displays a text log entry for the given commit. This change expands it to work much more like `git show`. Shows a commit (default `HEAD`) as a patch, in either text (a la `git show`) or JSON (with `--json`). HTML and 'quiet' output could be added in future. The patch is a diff plus some meta-information. The diff is just the same as the `sno diff` output for the relevant commit. In text mode, the meta-information is the same as what git show outputs: ``` commit ad9797ea83000864c2ab98c421d2d5256a12db8f Author: Craig de Stigter <craig@destigter.nz> Date: Wed Apr 15 13:19:16 2020 +1200 commit <diff output follows> ``` In JSON mode, the metadata is added to the top level JSON object so that the patch is also a diff. It looks like: ```json { "sno.diff/v1": {...}, "sno.patch/v1": { "authorName": "Craig de Stigter", "authorEmail": "craig@destigter.nz", "authorTime": "2020-04-15T01:19:16Z", "authorTimeOffset": "+12:00", "message": "commit" } } ```

olsen232 · 2020-04-20T00:59:14Z

+    help="Show commit in JSON patch format",
+    cls=MutexOption,
+    exclusive_with=["text"],
+)


Use

from .cli_util import do_json_option @do_json_option def show(ctx, *, refish, do_json, **kwargs):

Discussed out-of-band: going to leave this as-is because:

we're likely to add HTML output to this command shortly (we already have HTML diff output, so it's just a matter of adding a header with commit information)

the aliases -p and --patch are helpful (since JSON output is also the way you generate an applyable patch), so we don't want to remove those

craigds requested a review from rcoup April 15, 2020 08:44

rcoup reviewed Apr 15, 2020

View reviewed changes

craigds force-pushed the patch branch from c47b828 to e179e0d Compare April 15, 2020 22:18

craigds mentioned this pull request Apr 16, 2020

Add output pager to verbose commands #50

Open

craigds force-pushed the patch branch from e179e0d to a55e684 Compare April 16, 2020 21:19

craigds marked this pull request as ready for review April 16, 2020 22:42

craigds added 3 commits April 20, 2020 09:50

Refactor: move timestamp functions into their own file

ee2119b

Add some docstrings to diff output functions

089d0c8

diff: Split up large diff function into reusable parts

33b0958

Refactor output handling so it can be re-used by the show command.

craigds force-pushed the patch branch from a55e684 to 5b9be16 Compare April 19, 2020 21:52

craigds added 3 commits April 20, 2020 11:41

Minor error message tweaks

41f9ea1

Add test for show output

19b8d98

craigds force-pushed the patch branch from 5b9be16 to 19b8d98 Compare April 19, 2020 23:41

olsen232 reviewed Apr 20, 2020

View reviewed changes

Comment thread sno/diff.py Outdated

diff --html: Always open webbrowser with abs path

74e9117

olsen232 approved these changes Apr 20, 2020

View reviewed changes

craigds force-pushed the patch branch from 0f4bb55 to 74e9117 Compare April 20, 2020 01:27

craigds merged commit fed79a8 into master Apr 20, 2020

craigds deleted the patch branch April 20, 2020 01:27

craigds mentioned this pull request Apr 20, 2020

Allow diffs against the empty tree #53

Merged

Conversation

craigds commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To do

Output

Uh oh!

rcoup left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

craigds commented Apr 15, 2020

Uh oh!

rcoup commented Apr 16, 2020

Uh oh!

rcoup commented Apr 16, 2020

Uh oh!

craigds commented Apr 16, 2020

Uh oh!

craigds commented Apr 19, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

craigds commented Apr 15, 2020 •

edited

Loading